Repository navigation
Fix Swift 6 MainActor concurrency compatibility - #70
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR enables Aespa adoption on MainActor-isolated Swift 6 apps by adding nonisolated/Sendable annotations throughout the runtime, replacing OperationQueue with DispatchQueue-based work scheduling, introducing lock-guarded state synchronization, and ensuring all Combine event delivery occurs on the main thread. ChangesSwift 6 concurrency isolation and main-thread event delivery
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (11)
Sources/Aespa/AespaSession.swift (1)
355-362:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCall completion on successful session start.
The success path never invokes
onComplete, so callers depending on completion signaling won’t be notified when startup succeeds.💡 Suggested fix
func startSession(_ onComplete: `@escaping` CompletionHandler) { do { try coreSession.start() previewLayerSubject.sendOnMainThread(previewLayer) + onComplete(.success(())) } catch let error { onComplete(.failure(error)) } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/Aespa/AespaSession.swift` around lines 355 - 362, The startSession(_ onComplete: `@escaping` CompletionHandler) function currently only calls onComplete on error; after successfully calling try coreSession.start() and previewLayerSubject.sendOnMainThread(previewLayer) you must also invoke onComplete with a success result (e.g. onComplete(.success(()))) so callers are notified of successful startup; ensure you call the success completion on the same thread convention as failures (typically main thread) to match existing behavior.Sources/Aespa/Core/AespaCoreRecorder.swift (2)
24-35:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRecorder still bypasses
AespaCoreSession's serialized movie-output queue.Lines 25-32 read
core.movieFileOutputand callprocessor.process(output)directly from the caller's executor, even thoughAespaCoreSessionnow exposes a queuedrun<T: AespaMovieFileOutputProcessing>path for exactly this resource. That leavesstartRecording(...)outside the new serialization model.Suggested fix
func run<T: AespaMovieFileOutputProcessing>(processor: T, _ onComplete: `@escaping` CompletionHandler) { - guard let output = core.movieFileOutput else { - onComplete(.failure(AespaError.session(reason: .cannotFindConnection))) - return - } - - do { - try processor.process(output) - onComplete(.success(())) - } catch { - onComplete(.failure(error)) - } + core.run(processor, onComplete) }Also applies to: 39-50
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/Aespa/Core/AespaCoreRecorder.swift` around lines 24 - 35, The recorder's run<T: AespaMovieFileOutputProcessing>(processor:_:) is reading core.movieFileOutput and calling processor.process(output) directly, bypassing AespaCoreSession's queued execution; replace that direct access with a call into the session's serialized API (use the session's run<T: AespaMovieFileOutputProcessing>(...) or equivalent method exposed by AespaCoreSession) so the processor is executed on the core.movie-output queue, and do the same fix for the other similar overload (the block covering lines 39-50) to ensure all movieFileOutput operations go through the session's queued path.
52-66:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
stopRecording()can hang forever on synchronous stop failures.Line 53 discards the
run(processor:)result and then always awaits the subject. If the movie output is missing orFinishRecordProcessorfails beforedidFinishRecordingTofires, no value is ever sent and this continuation never resumes.Suggested fix
func stopRecording() async throws -> URL { - run(processor: FinishRecordProcessor(), { _ in }) - return try await withCheckedThrowingContinuation { continuation in fileIOResultSubsciption = fileIOResultSubject.sink { _ in // Do nothing on completion; we're only interested in values. } receiveValue: { result in switch result { case .success(let url): continuation.resume(returning: url) case .failure(let error): continuation.resume(throwing: error) } } + + run(processor: FinishRecordProcessor()) { result in + if case .failure(let error) = result { + self.fileIOResultSubsciption?.cancel() + self.fileIOResultSubsciption = nil + continuation.resume(throwing: error) + } + } } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/Aespa/Core/AespaCoreRecorder.swift` around lines 52 - 66, stopRecording() currently discards the synchronous result of run(processor: FinishRecordProcessor()) and then awaits fileIOResultSubject forever when run fails early; fix by subscribing to fileIOResultSubject (fileIOResultSubsciption) before calling run, and pass a completion handler to run(processor: FinishRecordProcessor(), ...) that immediately resumes the withCheckedThrowingContinuation with an error on synchronous failure (or forwards success), making sure to only call continuation.resume once (guard with a Bool or use continuation.resume(returning:) / resume(throwing:) exactly once). Ensure the completion handler or run's synchronous failure path and the subject's receiveValue both resume the same continuation and that you capture/clear fileIOResultSubsciption after resuming.Sources/Aespa/Core/AespaCoreCamera.swift (1)
16-17:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftSingle shared result channel breaks overlapping captures.
This type now advertises cross-task safety, but Lines 41-47 store every in-flight capture in one shared subscription. A second
capture(...)cancels the first sink, the first result is delivered to the wrong continuation, and the later result can double-resume the remaining continuation.Also applies to: 33-52, 56-69
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/Aespa/Core/AespaCoreCamera.swift` around lines 16 - 17, The bug is that a single shared PassthroughSubject (fileIOResultSubject) and a single subscription (fileIOResultSubsciption) are reused across overlapping capture(...) calls causing cancellations, mis-delivery, and double-resumes; fix by making the result channel per-capture (create a new PassthroughSubject<Result<AVCapturePhoto, Error>, Never> inside capture(...) or maintain a dictionary keyed by a captureID/UUID mapping to individual subjects/subscriptions), wire each capture to its own subscription so cancelling one doesn’t cancel others, and ensure each continuation is resumed only once by associating the continuation with that per-capture subject (update any code that references fileIOResultSubject and fileIOResultSubsciption to use the per-capture storage and keys).Sources/Aespa/Core/AespaCoreAlbumManager.swift (1)
12-22:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy lift
@unchecked Sendableis unsafe here without synchronizing the album state.Lines 65-89 and Lines 130-168 read and write
album,latestVideoFetchResult, andlatestPhotoFetchResultfrom multiple async entry points plus the Photos change callback, but this class has no lock or actor boundary. Two concurrent fetch/add calls can race album initialization, and observer updates can interleave with loader reads.Also applies to: 56-90, 129-168
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/Aespa/Core/AespaCoreAlbumManager.swift` around lines 12 - 22, AespaCoreAlbumManager currently marks itself `@unchecked` Sendable while reading/writing album, latestVideoFetchResult, and latestPhotoFetchResult from multiple async entry points and the Photos change callback; remove the unchecked Sendable marker and serialize access to those three properties (album, latestVideoFetchResult, latestPhotoFetchResult) by introducing a private synchronization primitive (e.g., a dedicated serial DispatchQueue or a concurrent queue with barrier writes) and ensure every read/write in the class—especially in the Photos change callback and in the fetch/add methods that initialize or update the album—occurs on that queue (use sync for reads and async/barrier for writes) so races are eliminated.Sources/Aespa/Core/AespaCoreSession.swift (1)
24-38:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not report success from
deferon throwable tuning paths.Line 30 and Line 51 execute before the
catchblock whentune(...)throws, so the same completion is invoked once with.successand then again with.failure. Any async bridge on top of this will trap on the second resume.Suggested fix
func run<T: AespaSessionTuning>(_ tuner: T, _ onComplete: `@escaping` CompletionHandler) { enqueueWork { do { if tuner.needTransaction { self.beginConfiguration() } defer { if tuner.needTransaction { self.commitConfiguration() } - onComplete(.success(())) } try tuner.tune(self) + onComplete(.success(())) } catch let error { Logger.log(error: error, message: "in \(tuner)") onComplete(.failure(error)) } } } func run<T: AespaDeviceTuning>(_ tuner: T, _ onComplete: `@escaping` CompletionHandler) { enqueueWork { do { guard let device = self.videoDeviceInput?.device else { throw AespaError.device(reason: .invalid) } if tuner.needLock { try device.lockForConfiguration() } defer { if tuner.needLock { device.unlockForConfiguration() } - onComplete(.success(())) } try tuner.tune(device) + onComplete(.success(())) } catch let error { Logger.log(error: error, message: "in \(tuner)") onComplete(.failure(error)) } } }Also applies to: 41-59
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/Aespa/Core/AespaCoreSession.swift` around lines 24 - 38, The defer currently calls commitConfiguration() and onComplete(.success(())) so when tuner.tune(self) throws the defer still reports success before the catch reports failure; fix by moving only commit/rollback configuration logic into the defer and remove onComplete(.success(())) from it: keep beginConfiguration() before try, have defer call commitConfiguration() (or rollback as appropriate) but call onComplete(.success(())) explicitly after a successful try (i.e., immediately after try tuner.tune(self) returns) and let the catch call onComplete(.failure(error)); update the run<T: AespaSessionTuning> function (referencing tuner.tune(self), beginConfiguration(), commitConfiguration(), and onComplete) accordingly so completion is invoked exactly once.Sources/Aespa/Core/Representable/Photos+AespaRepresentable.swift (1)
30-39:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDon't rewrite the caller's
PHFetchOptionsin place.
fetchAlbummutates thefetchOptionsinstance it was handed by overwriting its predicate. A reused options object will carry that hidden state into later fetches, andnonisolatedmakes accidental sharing across tasks easier. Build a local options object for the title filter instead of mutating the parameter.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/Aespa/Core/Representable/Photos`+AespaRepresentable.swift around lines 30 - 39, fetchAlbum currently mutates the caller's PHFetchOptions by assigning fetchOptions.predicate; instead create a new local PHFetchOptions (or copy the incoming one) and set the NSPredicate on that local instance, then pass the local options into PHAssetCollection.fetchAssetCollections; do not modify the parameter named fetchOptions, keep the function signature and return type (fetchAlbum<Collection: AespaAssetCollectionRepresentable>) unchanged and only replace the mutated options usage when calling PHAssetCollection.fetchAssetCollections.Sources/Aespa/Core/Representable/AespaCoreSession+AespaRepresentable.swift (2)
129-138:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove the attached video input, not a fresh wrapper.
removeMovieInput()creates a newAVCaptureDeviceInputand passes that toremoveInput(_:). That won't match the instance currently registered on the session, so the video input stays attached and later reconfiguration can fail.Suggested fix
func removeMovieInput() { - guard - let videoDevice = AVCaptureDevice.default(for: AVMediaType.video), - let videoInput = try? AVCaptureDeviceInput(device: videoDevice) - else { - return - } - - self.removeInput(videoInput) + if let videoDeviceInput { + self.removeInput(videoDeviceInput) + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/Aespa/Core/Representable/AespaCoreSession`+AespaRepresentable.swift around lines 129 - 138, removeMovieInput() builds a new AVCaptureDeviceInput and calls removeInput(_:) on that fresh wrapper, which won't match the already-attached instance; instead locate the existing AVCaptureDeviceInput in the session's inputs (e.g. iterate self.inputs or use first(where:)) whose device has mediaType == .video or whose device == AVCaptureDevice.default(for: .video) and pass that existing AVCaptureDeviceInput to removeInput(_:); update removeMovieInput() to find and remove the matching AVCaptureDeviceInput instance rather than creating a new one.
14-70:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftRoute
AVCaptureSessionstart/stop and input/output mutations through the session workQueue; fixremoveMovieInput()to remove the existing input instance.
AespaCoreSessionRepresentableandAespaCoreSessionarenonisolated, exposing directstartRunning/add/remove APIs. Separately,AespaSession.startSessioncallstry coreSession.start()insideTask, andAespaCoreSession.start()directly callssession.startRunning()+ adds inputs/outputs without going throughenqueueWork/workQueue, so the blocking + ordering constraints aren’t actually enforced for session start.removeMovieInput()constructs a brand-newAVCaptureDeviceInputand callsremoveInput(_:)with it; it should remove the currently-attached video input instance fromself.inputs(e.g., viavideoDeviceInput/ aninputslookup), matching howremoveAudioInput()behaves and avoiding identity-mismatch failures.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/Aespa/Core/Representable/AespaCoreSession`+AespaRepresentable.swift around lines 14 - 70, The start/stop and input/output mutations must be routed through the session workQueue/enqueueWork and removeMovieInput must remove the attached input instance instead of creating a new one: update AespaCoreSession.start() (and any direct calls to session.startRunning()/stopRunning(), addMovieInput(), addAudioInput(), addMovieFileOutput(), addCapturePhotoOutput()) to perform their session mutations inside enqueueWork/on the workQueue rather than calling AVCaptureSession APIs synchronously, and change removeMovieInput() to mirror removeAudioInput() by using the existing videoDeviceInput (or lookup from self.inputs) to call removeInput(_:) so you remove the actual attached AVCaptureDeviceInput instance instead of instantiating a new one.Sources/Aespa/Core/Context/AespaVideoContext.swift (1)
105-123:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClear
isRecordingbefore post-stop work can fail.After Line 108 succeeds, recording has already stopped, but
isRecordingis only reset after Lines 110-116 finish. Any failure in album import or file generation leaves the context stuck in the recording state even though the recorder is done.Suggested fix
Task(priority: .utility) { do { let videoFilePath = try await recorder.stopRecording() + isRecording = false if option.asset.synchronizeWithLocalAlbum { try await albumManager.addToAlbum(filePath: videoFilePath) // delete } @@ - isRecording = false onCompelte(.success(videoFile)) } catch let error { Logger.log(error: error) onCompelte(.failure(error)) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/Aespa/Core/Context/AespaVideoContext.swift` around lines 105 - 123, The recorder has already stopped when recorder.stopRecording() returns but isRecording is only set false after follow-up work; change stopRecording(_ onCompelte:) so that immediately after let videoFilePath = try await recorder.stopRecording() you set isRecording = false (before awaiting albumManager.addToAlbum or calling VideoFileGenerator.generate), then proceed with album import and file generation as before and ensure the existing catch still calls onCompelte(.failure(error)) and logs the error; keep references to recorder.stopRecording(), isRecording, albumManager.addToAlbum(filePath:), VideoFileGenerator.generate(with:), videoFileBufferSubject.sendOnMainThread(_:), and onCompelte to locate the changes.Sources/Aespa/Processor/File/FileWritingProcessor.swift (1)
14-17:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix the existence check before writing.
Line 16 passes
deletingLastPathComponent().absoluteStringintofileExists(atPath:), so this is checking afile://URL string instead of a filesystem path. It also checks the parent directory while throwing.alreadyExist, which makes the condition itself inconsistent. Usepath.path, and validate the exact thing you want to guard: the destination file for overwrite protection, or the parent directory for existence.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/Aespa/Processor/File/FileWritingProcessor.swift` around lines 14 - 17, In process(_ fileManager: FileManager) the current guard uses path.deletingLastPathComponent().absoluteString which passes a file:// URL string and checks the parent directory but throws AespaError.file(reason: .alreadyExist); change the logic to check the actual filesystem path using path.path and validate the intended target: if you want overwrite protection, call fileManager.fileExists(atPath: path.path) and throw AespaError.file(reason: .alreadyExist) when true; if instead you intend to validate the parent directory, call fileManager.fileExists(atPath: path.deletingLastPathComponent().path) and use a more appropriate error for a missing parent directory.
🧹 Nitpick comments (3)
Sources/Aespa/Data/Asset/PhotoAsset.swift (1)
14-20: ⚡ Quick winPrefer checked
Sendablehere if your target SDK already supports it.
PHAssetandUIImageareSendablein current Apple SDK docs, so@unchecked Sendablebuys nothing in that configuration and suppresses compiler checking for any future non-Sendable field added toPhotoAsset. If you no longer need an older SDK workaround, switch this to checkedSendable; otherwise, add a comment documenting the toolchain constraint. (developer.apple.com)Suggested change
-nonisolated public struct PhotoAsset: `@unchecked` Sendable { +nonisolated public struct PhotoAsset: Sendable {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/Aespa/Data/Asset/PhotoAsset.swift` around lines 14 - 20, Replace the unchecked sendable conformance on PhotoAsset: if your minimum toolchain already treats PHAsset and UIImage as Sendable, remove "`@unchecked`" and make PhotoAsset conform to checked Sendable (i.e. keep "nonisolated public struct PhotoAsset: Sendable") so the compiler enforces future safety for fields like asset: PHAsset and uiimage: UIImage; if you must support an older toolchain that lacks Sendable conformance for PHAsset/UIImage, keep "`@unchecked` Sendable" but add a clear comment above PhotoAsset documenting the toolchain constraint and why unchecked is required.Sources/Aespa/Data/File/PhotoFile.swift (1)
16-22: ⚡ Quick winUse checked
Sendablehere once the SDK baseline allows it.
UIImageisSendablein current Apple SDK docs, so this wrapper can likely use checkedSendableinstead of@unchecked Sendable. That keeps the public annotation explicit while preserving compiler enforcement if a future stored property isn't concurrency-safe. (developer.apple.com)Suggested change
-nonisolated public struct PhotoFile: `@unchecked` Sendable { +nonisolated public struct PhotoFile: Sendable {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/Aespa/Data/File/PhotoFile.swift` around lines 16 - 22, Change the concurrency annotation on the PhotoFile wrapper from unchecked to checked Sendable: update the declaration of nonisolated public struct PhotoFile: `@unchecked` Sendable to use nonisolated public struct PhotoFile: Sendable, keeping the stored properties (creationDate: Date and image: UIImage) as-is; this will rely on UIImage being Sendable in the SDK baseline and enable compiler enforcement for future non-thread-safe stored properties.Tests/Tests/Util/SubjectExtensionTests.swift (1)
20-58: ⚡ Quick winAdd a
CurrentValueSubjectcase to this suite.These tests only cover
PassthroughSubject, but the helper is also used withCurrentValueSubjectcall sites likeSources/Aespa/AespaSession.swift:23-37andSources/Aespa/Core/Context/AespaVideoContext.swift:18-32. A focused test there would protect the buffered/current-value path that motivated this PR.🧪 Example test shape
+ func testSendOnMainThread_withCurrentValueSubject_deliversOnMainThread() { + let subject = CurrentValueSubject<Int, Never>(0) + let expectation = expectation(description: "CurrentValueSubject delivery on main thread") + var receivedValue: Int? + var deliveredOnMainThread = false + + subject + .dropFirst() + .sink { value in + receivedValue = value + deliveredOnMainThread = Thread.isMainThread + expectation.fulfill() + } + .store(in: &cancellables) + + DispatchQueue.global(qos: .utility).async { + subject.sendOnMainThread(1) + } + + wait(for: [expectation], timeout: 2) + XCTAssertEqual(receivedValue, 1) + XCTAssertTrue(deliveredOnMainThread) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Tests/Tests/Util/SubjectExtensionTests.swift` around lines 20 - 58, Add two tests mirroring the PassthroughSubject cases but using CurrentValueSubject: implement testSendOnMainThread_whenCalledFromMainThread_withCurrentValue_deliversImmediately (create CurrentValueSubject<Int, Never>(0), subscribe with sink storing receivedValue and deliveredOnMainThread, call sendOnMainThread(1) and assert value and main-thread delivery) and testSendOnMainThread_whenCalledFromBackgroundThread_withCurrentValue_deliversOnMainThread (subscribe, call sendOnMainThread(2) from DispatchQueue.global, await expectation, then assert value and main-thread delivery). Use the same cancellables storage and assertion patterns as the existing PassthroughSubject tests and reference sendOnMainThread, CurrentValueSubject, and the existing test method names to locate where to add them.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/Aespa/Aespa.swift`:
- Around line 25-39: Currently the code assigns the new AespaSession to the
shared core inside withCoreLock before async authorization/start completes,
which can leave a stale failed singleton; instead, instantiate AespaSession
locally (do not assign to core), perform async authorization/start and only
inside withCoreLock assign core = newCore after the start succeeds, and on
failure ensure you do not persist core and propagate the .failure via the
existing session(...) completion path; apply the same pattern for the similar
block referenced (lines 45-53) and use the same symbols AespaSession, core,
withCoreLock, shouldConfigureSession, and session(...) to locate and update the
logic.
In `@Sources/Aespa/Core/AespaCoreSession.swift`:
- Around line 94-106: The start() method can throw during
addMovieInput/addMovieFileOutput/addCapturePhotoOutput and currently never
activates the work queue or fails pending run(...) closures; change start() to
ensure activateWorkQueue() is always called (use defer or a do/catch that calls
activateWorkQueue() in both paths) and, on error, drain pendingWorkItems and
complete each queued closure with a failure (propagate the thrown Error) so
callers waiting on run(...) are resumed with an error; reference start(),
activateWorkQueue(), pendingWorkItems, run(...), addMovieInput(),
addMovieFileOutput(), and addCapturePhotoOutput() when implementing.
In `@Sources/Aespa/Core/Context/AespaPhotoContext.swift`:
- Around line 114-117: The code stores the caller-provided
AVCapturePhotoSettings instance directly into photoSetting (inside the case
.custom branch and the other assignment where photoSetting = setting), which
allows external mutation races; instead, make a defensive copy before storing by
creating a new AVCapturePhotoSettings from the provided one (e.g. via
AVCapturePhotoSettings(from:)) and assign that copy inside withSettingLock so
photoSetting always holds an independent instance; update both places that
assign photoSetting (the case .custom branch and the other assignment site) to
use the copied instance.
In `@Sources/Aespa/Util/Log/Logger.swift`:
- Around line 10-11: Logger.enableLogging is a mutable static flag accessed
concurrently (written in Aespa.session(with:onComplete:) and read in Logger.log
running on AespaCoreSession.workQueue), causing a potential data race; fix by
making the flag immutable after initial setup or syncrhonize access: either (a)
move logging enablement into per-session configuration passed into
AespaCoreSession and remove the static, (b) make enableLogging a let or an
once-set property that refuses subsequent writes, or (c) guard reads/writes with
a lock or atomic wrapper around Logger.enableLogging; update references in
Logger (enableLogging, log) and the writer in Aespa.session(with:onComplete:)
accordingly so all accesses are thread-safe.
---
Outside diff comments:
In `@Sources/Aespa/AespaSession.swift`:
- Around line 355-362: The startSession(_ onComplete: `@escaping`
CompletionHandler) function currently only calls onComplete on error; after
successfully calling try coreSession.start() and
previewLayerSubject.sendOnMainThread(previewLayer) you must also invoke
onComplete with a success result (e.g. onComplete(.success(()))) so callers are
notified of successful startup; ensure you call the success completion on the
same thread convention as failures (typically main thread) to match existing
behavior.
In `@Sources/Aespa/Core/AespaCoreAlbumManager.swift`:
- Around line 12-22: AespaCoreAlbumManager currently marks itself `@unchecked`
Sendable while reading/writing album, latestVideoFetchResult, and
latestPhotoFetchResult from multiple async entry points and the Photos change
callback; remove the unchecked Sendable marker and serialize access to those
three properties (album, latestVideoFetchResult, latestPhotoFetchResult) by
introducing a private synchronization primitive (e.g., a dedicated serial
DispatchQueue or a concurrent queue with barrier writes) and ensure every
read/write in the class—especially in the Photos change callback and in the
fetch/add methods that initialize or update the album—occurs on that queue (use
sync for reads and async/barrier for writes) so races are eliminated.
In `@Sources/Aespa/Core/AespaCoreCamera.swift`:
- Around line 16-17: The bug is that a single shared PassthroughSubject
(fileIOResultSubject) and a single subscription (fileIOResultSubsciption) are
reused across overlapping capture(...) calls causing cancellations,
mis-delivery, and double-resumes; fix by making the result channel per-capture
(create a new PassthroughSubject<Result<AVCapturePhoto, Error>, Never> inside
capture(...) or maintain a dictionary keyed by a captureID/UUID mapping to
individual subjects/subscriptions), wire each capture to its own subscription so
cancelling one doesn’t cancel others, and ensure each continuation is resumed
only once by associating the continuation with that per-capture subject (update
any code that references fileIOResultSubject and fileIOResultSubsciption to use
the per-capture storage and keys).
In `@Sources/Aespa/Core/AespaCoreRecorder.swift`:
- Around line 24-35: The recorder's run<T:
AespaMovieFileOutputProcessing>(processor:_:) is reading core.movieFileOutput
and calling processor.process(output) directly, bypassing AespaCoreSession's
queued execution; replace that direct access with a call into the session's
serialized API (use the session's run<T: AespaMovieFileOutputProcessing>(...) or
equivalent method exposed by AespaCoreSession) so the processor is executed on
the core.movie-output queue, and do the same fix for the other similar overload
(the block covering lines 39-50) to ensure all movieFileOutput operations go
through the session's queued path.
- Around line 52-66: stopRecording() currently discards the synchronous result
of run(processor: FinishRecordProcessor()) and then awaits fileIOResultSubject
forever when run fails early; fix by subscribing to fileIOResultSubject
(fileIOResultSubsciption) before calling run, and pass a completion handler to
run(processor: FinishRecordProcessor(), ...) that immediately resumes the
withCheckedThrowingContinuation with an error on synchronous failure (or
forwards success), making sure to only call continuation.resume once (guard with
a Bool or use continuation.resume(returning:) / resume(throwing:) exactly once).
Ensure the completion handler or run's synchronous failure path and the
subject's receiveValue both resume the same continuation and that you
capture/clear fileIOResultSubsciption after resuming.
In `@Sources/Aespa/Core/AespaCoreSession.swift`:
- Around line 24-38: The defer currently calls commitConfiguration() and
onComplete(.success(())) so when tuner.tune(self) throws the defer still reports
success before the catch reports failure; fix by moving only commit/rollback
configuration logic into the defer and remove onComplete(.success(())) from it:
keep beginConfiguration() before try, have defer call commitConfiguration() (or
rollback as appropriate) but call onComplete(.success(())) explicitly after a
successful try (i.e., immediately after try tuner.tune(self) returns) and let
the catch call onComplete(.failure(error)); update the run<T:
AespaSessionTuning> function (referencing tuner.tune(self),
beginConfiguration(), commitConfiguration(), and onComplete) accordingly so
completion is invoked exactly once.
In `@Sources/Aespa/Core/Context/AespaVideoContext.swift`:
- Around line 105-123: The recorder has already stopped when
recorder.stopRecording() returns but isRecording is only set false after
follow-up work; change stopRecording(_ onCompelte:) so that immediately after
let videoFilePath = try await recorder.stopRecording() you set isRecording =
false (before awaiting albumManager.addToAlbum or calling
VideoFileGenerator.generate), then proceed with album import and file generation
as before and ensure the existing catch still calls onCompelte(.failure(error))
and logs the error; keep references to recorder.stopRecording(), isRecording,
albumManager.addToAlbum(filePath:), VideoFileGenerator.generate(with:),
videoFileBufferSubject.sendOnMainThread(_:), and onCompelte to locate the
changes.
In `@Sources/Aespa/Core/Representable/AespaCoreSession`+AespaRepresentable.swift:
- Around line 129-138: removeMovieInput() builds a new AVCaptureDeviceInput and
calls removeInput(_:) on that fresh wrapper, which won't match the
already-attached instance; instead locate the existing AVCaptureDeviceInput in
the session's inputs (e.g. iterate self.inputs or use first(where:)) whose
device has mediaType == .video or whose device == AVCaptureDevice.default(for:
.video) and pass that existing AVCaptureDeviceInput to removeInput(_:); update
removeMovieInput() to find and remove the matching AVCaptureDeviceInput instance
rather than creating a new one.
- Around line 14-70: The start/stop and input/output mutations must be routed
through the session workQueue/enqueueWork and removeMovieInput must remove the
attached input instance instead of creating a new one: update
AespaCoreSession.start() (and any direct calls to
session.startRunning()/stopRunning(), addMovieInput(), addAudioInput(),
addMovieFileOutput(), addCapturePhotoOutput()) to perform their session
mutations inside enqueueWork/on the workQueue rather than calling
AVCaptureSession APIs synchronously, and change removeMovieInput() to mirror
removeAudioInput() by using the existing videoDeviceInput (or lookup from
self.inputs) to call removeInput(_:) so you remove the actual attached
AVCaptureDeviceInput instance instead of instantiating a new one.
In `@Sources/Aespa/Core/Representable/Photos`+AespaRepresentable.swift:
- Around line 30-39: fetchAlbum currently mutates the caller's PHFetchOptions by
assigning fetchOptions.predicate; instead create a new local PHFetchOptions (or
copy the incoming one) and set the NSPredicate on that local instance, then pass
the local options into PHAssetCollection.fetchAssetCollections; do not modify
the parameter named fetchOptions, keep the function signature and return type
(fetchAlbum<Collection: AespaAssetCollectionRepresentable>) unchanged and only
replace the mutated options usage when calling
PHAssetCollection.fetchAssetCollections.
In `@Sources/Aespa/Processor/File/FileWritingProcessor.swift`:
- Around line 14-17: In process(_ fileManager: FileManager) the current guard
uses path.deletingLastPathComponent().absoluteString which passes a file:// URL
string and checks the parent directory but throws AespaError.file(reason:
.alreadyExist); change the logic to check the actual filesystem path using
path.path and validate the intended target: if you want overwrite protection,
call fileManager.fileExists(atPath: path.path) and throw AespaError.file(reason:
.alreadyExist) when true; if instead you intend to validate the parent
directory, call fileManager.fileExists(atPath:
path.deletingLastPathComponent().path) and use a more appropriate error for a
missing parent directory.
---
Nitpick comments:
In `@Sources/Aespa/Data/Asset/PhotoAsset.swift`:
- Around line 14-20: Replace the unchecked sendable conformance on PhotoAsset:
if your minimum toolchain already treats PHAsset and UIImage as Sendable, remove
"`@unchecked`" and make PhotoAsset conform to checked Sendable (i.e. keep
"nonisolated public struct PhotoAsset: Sendable") so the compiler enforces
future safety for fields like asset: PHAsset and uiimage: UIImage; if you must
support an older toolchain that lacks Sendable conformance for PHAsset/UIImage,
keep "`@unchecked` Sendable" but add a clear comment above PhotoAsset documenting
the toolchain constraint and why unchecked is required.
In `@Sources/Aespa/Data/File/PhotoFile.swift`:
- Around line 16-22: Change the concurrency annotation on the PhotoFile wrapper
from unchecked to checked Sendable: update the declaration of nonisolated public
struct PhotoFile: `@unchecked` Sendable to use nonisolated public struct
PhotoFile: Sendable, keeping the stored properties (creationDate: Date and
image: UIImage) as-is; this will rely on UIImage being Sendable in the SDK
baseline and enable compiler enforcement for future non-thread-safe stored
properties.
In `@Tests/Tests/Util/SubjectExtensionTests.swift`:
- Around line 20-58: Add two tests mirroring the PassthroughSubject cases but
using CurrentValueSubject: implement
testSendOnMainThread_whenCalledFromMainThread_withCurrentValue_deliversImmediately
(create CurrentValueSubject<Int, Never>(0), subscribe with sink storing
receivedValue and deliveredOnMainThread, call sendOnMainThread(1) and assert
value and main-thread delivery) and
testSendOnMainThread_whenCalledFromBackgroundThread_withCurrentValue_deliversOnMainThread
(subscribe, call sendOnMainThread(2) from DispatchQueue.global, await
expectation, then assert value and main-thread delivery). Use the same
cancellables storage and assertion patterns as the existing PassthroughSubject
tests and reference sendOnMainThread, CurrentValueSubject, and the existing test
method names to locate where to add them.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 57fb9daa-9c6d-434c-8265-6cd2da329e1a
📒 Files selected for processing (45)
.gitignoreScripts/gen-mocks.shScripts/versionSources/Aespa/Aespa.swiftSources/Aespa/AespaOption.swiftSources/Aespa/AespaSession.swiftSources/Aespa/Core/AespaCoreAlbumManager.swiftSources/Aespa/Core/AespaCoreCamera.swiftSources/Aespa/Core/AespaCoreRecorder.swiftSources/Aespa/Core/AespaCoreSession.swiftSources/Aespa/Core/AespaEventManager.swiftSources/Aespa/Core/Context/AespaPhotoContext.swiftSources/Aespa/Core/Context/AespaVideoContext.swiftSources/Aespa/Core/Context/Context.swiftSources/Aespa/Core/Representable/AVCaptureConnection+AespaRepresentable.swiftSources/Aespa/Core/Representable/AVCaptureDevice+AespaRepresentable.swiftSources/Aespa/Core/Representable/AVCaptureFileOutput+AespaRepresentable.swiftSources/Aespa/Core/Representable/AVCapturePhotoOutput+AespaRepresentable.swiftSources/Aespa/Core/Representable/AespaCoreSession+AespaRepresentable.swiftSources/Aespa/Core/Representable/Photos+AespaRepresentable.swiftSources/Aespa/Data/Asset/PhotoAsset.swiftSources/Aespa/Data/Asset/Video�Asset.swiftSources/Aespa/Data/File/PhotoFile.swiftSources/Aespa/Data/File/VideoFile.swiftSources/Aespa/Data/Proxy/AssetCachingProxy.swiftSources/Aespa/Loader/AespaLoading.swiftSources/Aespa/Loader/Asset/AssetLoader.swiftSources/Aespa/Processor/AespaProcessing.swiftSources/Aespa/Processor/Asset/PhotoAssetAdditionProcessor.swiftSources/Aespa/Processor/Asset/VideoAssetAdditionProcessor.swiftSources/Aespa/Processor/Capture/CapturePhotoProcessor.swiftSources/Aespa/Processor/File/FileWritingProcessor.swiftSources/Aespa/Processor/Record/FinishRecordProcessor.swiftSources/Aespa/Processor/Record/StartRecordProcessor.swiftSources/Aespa/Tuner/AespaTuning.swiftSources/Aespa/Util/Album/AlbumImporter.swiftSources/Aespa/Util/Extension/AVFoundation+Extension.swiftSources/Aespa/Util/Extension/Subject+Extension.swiftSources/Aespa/Util/Extension/UIKit+Extension.swiftSources/Aespa/Util/File/FilePathProvider.swiftSources/Aespa/Util/File/PhotoFileGenerator.swiftSources/Aespa/Util/File/VideoFileGenerator.swiftSources/Aespa/Util/Log/Logger.swiftTests/TestHostApp.xcodeproj/project.pbxprojTests/Tests/Util/SubjectExtensionTests.swift
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/Aespa/Core/Context/AespaVideoContext.swift (1)
105-125:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTypo in parameter name:
onCompelteshould beonComplete.Line 105 has
onCompelteinstead ofonComplete. This is a public API, so the typo will be visible to consumers.✏️ Proposed fix
- public func stopRecording(_ onCompelte: `@escaping` ResultHandler<VideoFile> = { _ in }) { + public func stopRecording(_ onComplete: `@escaping` ResultHandler<VideoFile> = { _ in }) { Task(priority: .utility) { do { let videoFilePath = try await recorder.stopRecording() isRecording = false if option.asset.synchronizeWithLocalAlbum { try await albumManager.addToAlbum(filePath: videoFilePath) // delete } let videoFile = VideoFileGenerator.generate(with: videoFilePath, date: Date()) videoFileBufferSubject.sendOnMainThread(.success(videoFile)) - onCompelte(.success(videoFile)) + onComplete(.success(videoFile)) } catch let error { Logger.log(error: error) - onCompelte(.failure(error)) + onComplete(.failure(error)) } } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/Aespa/Core/Context/AespaVideoContext.swift` around lines 105 - 125, Rename the misspelled public completion parameter in the stopRecording method from `onCompelte` to `onComplete` and update all internal uses inside the method (the calls `onCompelte(.success(videoFile))` and `onCompelte(.failure(error))`) to the corrected name; ensure the method signature in `AespaVideoContext.stopRecording(_:)` preserves the same default closure value and behavior so external callers get the corrected API name, and update any callers/tests that referenced `onCompelte` to use `onComplete`.
🧹 Nitpick comments (1)
Sources/Aespa/Core/AespaCoreCamera.swift (1)
51-71: 💤 Low valueConsider adding a comment explaining the
nonisolated(unsafe)usage.Line 67 uses
nonisolated(unsafe)to work aroundAVCapturePhotonot conforming toSendable. While this is a pragmatic workaround for Swift 6 compatibility, documenting why this is safe (the photo is consumed immediately and not shared across isolation domains) would help future maintainers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/Aespa/Core/AespaCoreCamera.swift` around lines 51 - 71, Add a short inline comment above the nonisolated(unsafe) let uncheckedPhoto = photo in the nonisolated extension AespaCoreCamera's photoOutput(_:didFinishProcessingPhoto:error:) explaining why the unsafe nonisolated cast is safe: note that AVCapturePhoto is not Sendable, but here the photo is consumed immediately on the same thread, the associated continuation removed via removeContinuation(for:) prevents cross-isolation sharing, and continuation.resume(returning:) returns the photo synchronously so it won't escape the actor boundary; keep the comment succinct and reference these symbols (nonisolated extension AespaCoreCamera, photoOutput(_:didFinishProcessingPhoto:error:), removeContinuation(for:), continuation.resume(returning:)) for future maintainers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@Sources/Aespa/Core/Context/AespaVideoContext.swift`:
- Around line 105-125: Rename the misspelled public completion parameter in the
stopRecording method from `onCompelte` to `onComplete` and update all internal
uses inside the method (the calls `onCompelte(.success(videoFile))` and
`onCompelte(.failure(error))`) to the corrected name; ensure the method
signature in `AespaVideoContext.stopRecording(_:)` preserves the same default
closure value and behavior so external callers get the corrected API name, and
update any callers/tests that referenced `onCompelte` to use `onComplete`.
---
Nitpick comments:
In `@Sources/Aespa/Core/AespaCoreCamera.swift`:
- Around line 51-71: Add a short inline comment above the nonisolated(unsafe)
let uncheckedPhoto = photo in the nonisolated extension AespaCoreCamera's
photoOutput(_:didFinishProcessingPhoto:error:) explaining why the unsafe
nonisolated cast is safe: note that AVCapturePhoto is not Sendable, but here the
photo is consumed immediately on the same thread, the associated continuation
removed via removeContinuation(for:) prevents cross-isolation sharing, and
continuation.resume(returning:) returns the photo synchronously so it won't
escape the actor boundary; keep the comment succinct and reference these symbols
(nonisolated extension AespaCoreCamera,
photoOutput(_:didFinishProcessingPhoto:error:), removeContinuation(for:),
continuation.resume(returning:)) for future maintainers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: aeef1e73-1825-4ec6-a07b-d0b9634d65c8
📒 Files selected for processing (11)
Sources/Aespa/Aespa.swiftSources/Aespa/AespaSession.swiftSources/Aespa/Core/AespaCoreAlbumManager.swiftSources/Aespa/Core/AespaCoreCamera.swiftSources/Aespa/Core/AespaCoreRecorder.swiftSources/Aespa/Core/AespaCoreSession.swiftSources/Aespa/Core/Context/AespaPhotoContext.swiftSources/Aespa/Core/Context/AespaVideoContext.swiftSources/Aespa/Core/Representable/AespaCoreSession+AespaRepresentable.swiftSources/Aespa/Data/Asset/VideoAsset.swiftSources/Aespa/Util/Log/Logger.swift
💤 Files with no reviewable changes (1)
- Sources/Aespa/Data/Asset/VideoAsset.swift
🚧 Files skipped from review as they are similar to previous changes (5)
- Sources/Aespa/Util/Log/Logger.swift
- Sources/Aespa/Core/AespaCoreSession.swift
- Sources/Aespa/Aespa.swift
- Sources/Aespa/AespaSession.swift
- Sources/Aespa/Core/Context/AespaPhotoContext.swift
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
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
Related Issues
Verification
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores
Tests