Conversation
…d OS and remove remaining Big Sur things
With SWIFT_DEFAULT_ACTOR_ISOLATION = MainActor, all types are MainActor by default. The actor's synchronization purpose (single-flight token refresh) is now provided by MainActor serialization. Changes: - actor NetworkAuthManager → class NetworkAuthManager - bearerAuthSupported(): drop async (no longer needed without actor) - basicAuthString(): drop nonisolated (no longer an actor) - JamfProAPIClient: drop await from bearerAuthSupported() calls - NetworkAuthManagerTests: drop await from bearerAuthSupported() calls Side effect: Resolves Token.isValid cross-isolation warning because Token and NetworkAuthManager are now both on MainActor. Agent-Logs-Url: https://github.com/jamf/PPPC-Utility/sessions/4235dbf5-feb0-4744-839d-543eee3b8ee5 Co-authored-by: watkyn <40115+watkyn@users.noreply.github.com>
With MainActor default isolation, manual dispatch to the main thread is redundant. All three locations are already MainActor-isolated. Changes: - Alert.swift: Remove DispatchQueue.main.async wrapper in display() - OpenViewController.swift: Remove wrapper in tableView selectionIndexes - SaveViewController.swift: Remove wrapper in savePressed panel callback Agent-Logs-Url: https://github.com/jamf/PPPC-Utility/sessions/4235dbf5-feb0-4744-839d-543eee3b8ee5 Co-authored-by: watkyn <40115+watkyn@users.noreply.github.com>
Replace Task/completion handler pattern with direct async throws methods.
Changes:
- UploadManager.verifyConnection: async throws -> VerificationInfo
- UploadManager.upload: async throws (no completion handler)
- UploadInfoView.verifyConnection: now async, uses try await
- UploadInfoView.performUpload: now async, uses try await
- Button action bridges to async with Task { await ... }
Agent-Logs-Url: https://github.com/jamf/PPPC-Utility/sessions/4235dbf5-feb0-4744-839d-543eee3b8ee5
Co-authored-by: watkyn <40115+watkyn@users.noreply.github.com>
Replace completion handler pattern with throws -> Executable since the work is synchronous (reads bundle info and code requirements). Changes: - Model.loadExecutable(url:) now throws -> Executable (no completion) - Remove LoadExecutableCompletion typealias - findExecutableOnComputerUsing → findExecutable, returns directly - getExecutableFrom: uses do/catch instead of completion handler - getAppleEventChoices: uses do/catch for each loadExecutable call - TCCProfileViewController: promptForExecutables and acceptDrop updated - OpenViewController.prompt: updated to use try/catch Agent-Logs-Url: https://github.com/jamf/PPPC-Utility/sessions/4235dbf5-feb0-4744-839d-543eee3b8ee5 Co-authored-by: watkyn <40115+watkyn@users.noreply.github.com>
Replace completion handler pattern with throws -> TCCProfile since the work is synchronous (decode data, read file). Changes: - TCCProfileImporter.decodeTCCProfile(data:) now throws -> TCCProfile - TCCProfileImporter.decodeTCCProfile(fileUrl:) now throws -> TCCProfile - Remove TCCProfileImportCompletion typealias - TCCProfileConfigurationPanel: uses try/catch in panel callback - TCCProfileImporterTests: updated to use try/catch pattern Agent-Logs-Url: https://github.com/jamf/PPPC-Utility/sessions/4235dbf5-feb0-4744-839d-543eee3b8ee5 Co-authored-by: watkyn <40115+watkyn@users.noreply.github.com>
Mark SecurityWrapper and Model methods with @Concurrent to move disk I/O and security operations off MainActor onto the cooperative pool. Changes: - SecurityWrapper.copyDesignatedRequirement: @Concurrent async throws - SecurityWrapper.sign: @Concurrent async throws - SecurityWrapper.loadSigningIdentities: @Concurrent async throws - Model.loadExecutable: @Concurrent async throws -> Executable - Model.getAppleEventChoices: now async (calls loadExecutable) - Model.getExecutableFrom/findExecutable: now async - Model.getExecutablesFromAllPolicies: now async - Model.importProfile: now async - All callers updated to use Task/await where needed - ModelTests: updated to async test methods Agent-Logs-Url: https://github.com/jamf/PPPC-Utility/sessions/4235dbf5-feb0-4744-839d-543eee3b8ee5 Co-authored-by: watkyn <40115+watkyn@users.noreply.github.com>
Flip SWIFT_VERSION from 5.0 to 6.0 so all concurrency warnings become hard errors. Fix override isolation mismatches. Changes: - SWIFT_VERSION = 6.0 in all 4 build configurations - SaveViewController.observeValue: add nonisolated + MainActor.assumeIsolated - ModelTests.setUp: add nonisolated + MainActor.assumeIsolated - Update plan document to reflect all stages complete Agent-Logs-Url: https://github.com/jamf/PPPC-Utility/sessions/4235dbf5-feb0-4744-839d-543eee3b8ee5 Co-authored-by: watkyn <40115+watkyn@users.noreply.github.com>
…fix Unit Test issues
…eate-separate-pull-requests
There was a problem hiding this comment.
Pull request overview
This PR moves the app toward Swift’s “approachable concurrency” model by enabling MainActor default isolation and refactoring several callback-based APIs to async/await, with the goal of reducing explicit main-thread dispatch and tightening concurrency guarantees.
Changes:
- Enable strict concurrency / MainActor default isolation in Xcode build settings and switch the app target to Swift 6 language mode.
- Convert multiple completion-handler flows (uploading, profile importing, executable loading) to
async/async throws. - Update UI controllers and SwiftUI views to use
Task {}bridges at UI entry points andawaitthe new async APIs.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| Source/Views/Alert.swift | Removes explicit DispatchQueue.main.async wrapper around alert display. |
| Source/View Controllers/TCCProfileViewController.swift | Refactors identity loading/importing/executable loading to async/await; adjusts drag-drop handling. |
| Source/View Controllers/SaveViewController.swift | Loads identities and signs/saves profiles via async APIs; adjusts KVO override isolation. |
| Source/View Controllers/OpenViewController.swift | Makes choices/executable loading async and updates selection handling. |
| Source/TCCProfileImporter/TCCProfileImporter.swift | Converts decoding APIs from completion-handler results to throws returns. |
| Source/TCCProfileImporter/TCCProfileConfigurationPanel.swift | Makes open-panel flow async throws returning an optional profile. |
| Source/SwiftUI/UploadInfoView.swift | Refactors verify/upload flows to async and bridges from button actions with Task {}. |
| Source/SecurityWrapper.swift | Introduces async security operations and adds concurrency-related annotations/imports. |
| Source/Networking/UploadManager.swift | Converts verify/upload APIs to async throws (no internal Task/completion callbacks). |
| Source/Networking/NetworkAuthManager.swift | Converts NetworkAuthManager from actor to class under MainActor default isolation. |
| Source/Networking/JamfProAPIClient.swift | Removes now-unnecessary await for bearerAuthSupported(). |
| Source/Model/TCCProfile.swift | Makes Jamf Pro API data generation async to support async signing. |
| Source/Model/SigningIdentity.swift | Adds concurrency opt-outs to allow construction/access across isolation contexts. |
| Source/Model/Model.swift | Converts executable loading and several dependent flows to async/await. |
| Source/AppDelegate.swift | Migrates from @NSApplicationMain to @main. |
| PPPC UtilityTests/TCCProfileImporterTests/TCCProfileTests.swift | Updates tests for async JamfPro API data generation and signature changes. |
| PPPC UtilityTests/TCCProfileImporterTests/TCCProfileImporterTests.swift | Updates importer tests for new throwing decode APIs. |
| PPPC UtilityTests/NetworkingTests/NetworkAuthManagerTests.swift | Updates tests for bearerAuthSupported() no longer being async. |
| PPPC UtilityTests/ModelTests/ModelTests.swift | Updates model tests for async model APIs and minor comment header rename. |
| PPPC Utility.xcodeproj/project.pbxproj | Enables strict concurrency settings; switches app target to Swift 6 language mode. |
| docs/plans/approachable-concurrency.md | Adds a staged migration plan for adopting approachable concurrency / Swift 6. |
| CLAUDE.md | Renames Swift Testing section heading and clarifies conventions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| nonisolated override func observeValue(forKeyPath keyPath: String?, of object: Any?, change: [NSKeyValueChangeKey: Any]?, context: UnsafeMutableRawPointer?) { | ||
| if context == &SaveViewController.saveProfileKVOContext { | ||
| updateIsReadyToSave() | ||
| MainActor.assumeIsolated { |
There was a problem hiding this comment.
observeValue(...) is marked nonisolated, but it uses MainActor.assumeIsolated { ... }. KVO callbacks are not guaranteed to arrive on the MainActor, so this can trigger a runtime isolation precondition failure/crash if the notification is delivered off-main. Prefer hopping explicitly to the MainActor (e.g., Task { @MainActor in ... } / await MainActor.run { ... }) instead of assumeIsolated.
| MainActor.assumeIsolated { | |
| Task { @MainActor in |
There was a problem hiding this comment.
I think for this UI project it's safe to assume that changes to user defaults are always gonna arrive on the main thread so I think we should ignore this suggestion.
| @objc dynamic var displayName: String | ||
| var reference: SecIdentity? | ||
| nonisolated(unsafe) var reference: SecIdentity? | ||
|
|
||
| init(name: String, reference: SecIdentity?) { | ||
| displayName = name | ||
| super.init() | ||
| nonisolated init(name: String, reference: SecIdentity?) { | ||
| self.displayName = name | ||
| self.reference = reference | ||
| super.init() |
There was a problem hiding this comment.
reference is declared as nonisolated(unsafe) var, but it’s only assigned in init and never mutated afterwards. Keeping this mutable + unsafe widens the surface for data races. Consider making it immutable (let) (and/or limiting mutation with private(set)) to avoid needing an unsafe opt-out here.
| @@ -97,11 +97,11 @@ struct SecurityWrapper { | |||
| return outputData! as Data | |||
| } | |||
There was a problem hiding this comment.
sign(data:using:) does synchronous CMS signing work but is now async without being offloaded from MainActor. With SWIFT_DEFAULT_ACTOR_ISOLATION = MainActor, this will still execute on the UI executor and can block the app during signing. Consider moving this work off the MainActor (e.g., marking it @concurrent like copyDesignatedRequirement, or otherwise performing the signing on a background executor) and returning the result back to the caller.
| authMgr: makeAuthManager(), | ||
| siteInfo: siteIdAndName, | ||
| signingIdentity: mustSign ? signingId : nil) | ||
| Alert().display(header: "Success", message: "Profile uploaded succesfully") |
There was a problem hiding this comment.
Typo in user-facing success message: “succesfully” → “successfully”.
| Alert().display(header: "Success", message: "Profile uploaded succesfully") | |
| Alert().display(header: "Success", message: "Profile uploaded successfully") |
| PRODUCT_BUNDLE_IDENTIFIER = "com.jamf.PPPC-UtilityTests"; | ||
| PRODUCT_NAME = "$(TARGET_NAME)"; | ||
| PROVISIONING_PROFILE_SPECIFIER = ""; | ||
| SWIFT_APPROACHABLE_CONCURRENCY = YES; | ||
| SWIFT_DEFAULT_ACTOR_ISOLATION = MainActor; | ||
| SWIFT_OPTIMIZATION_LEVEL = "-Onone"; | ||
| SWIFT_STRICT_CONCURRENCY = complete; | ||
| SWIFT_VERSION = 5.0; | ||
| TEST_HOST = "$(BUILT_PRODUCTS_DIR)/PPPC Utility.app/Contents/MacOS/PPPC Utility"; |
There was a problem hiding this comment.
The app target is switched to SWIFT_VERSION = 6.0 but the test target remains on Swift 5.0. This can allow concurrency issues to slip through tests (warnings vs errors) and makes compiler behavior differ between targets. If the goal is full strict concurrency enforcement, align the test target’s Swift version with the app target.
| guard let urls = urls, !urls.isEmpty else { return false } | ||
|
|
||
| Task { | ||
| for url in urls { | ||
| do { | ||
| let newExecutable = try await model.loadExecutable(url: url) | ||
| if tableView == self.executablesTable { | ||
| guard self.executablesAC.canInsert else { | ||
| continue | ||
| } | ||
| if self?.shouldExecutableBeAdded(newExecutable) ?? false { | ||
| self?.executablesAC.insert(newExecutable, atArrangedObjectIndex: row) | ||
| addedAny = true | ||
| if self.shouldExecutableBeAdded(newExecutable) { | ||
| self.executablesAC.insert(newExecutable, atArrangedObjectIndex: row) | ||
| } | ||
| } else { | ||
| self?.insertIntoAppleEvents(newExecutable) | ||
| addedAny = true | ||
| self.insertIntoAppleEvents(newExecutable) | ||
| } | ||
| } catch { | ||
| if let loadError = error as? LoadExecutableError { | ||
| self.showAlert(loadError, for: window) | ||
| } | ||
| case .failure(let error): | ||
| self?.showAlert(error, for: window) | ||
| self?.logger.error("\(error)") | ||
| self.logger.error("\(error)") | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return addedAny | ||
| return true | ||
| } |
There was a problem hiding this comment.
acceptDrop now returns true immediately even when the destination controller can’t accept inserts (e.g., executablesAC.canInsert == false), which can cause AppKit to treat the drop as handled even though nothing will be inserted. Consider checking canInsert (and any other immediate prerequisites) before starting the async work and returning false when the drop can’t be accepted.
There was a problem hiding this comment.
Copilot's comment here is correct. Previous behavior checked all of the things synchronously to know if the drop can be accepted. Putting it in a Task moves most of that to after the table already has accepted the drop.
|
|
||
| /// This actor ensures that only one token refresh occurs at the same time. | ||
| actor NetworkAuthManager { | ||
| /// This class ensures that only one token refresh occurs at the same time. |
There was a problem hiding this comment.
I think this change to the NetworkAuthManager from actor to class should be reverted. The NetworkAuthManager's whole concept is to serialize token refresh, even if networking happens in the background or multiple network calls are ongoing concurrently.
If the networking code is ever changed to happen in the background and you don't want to tie up the main thread with network synchronization issues, or if this class is copy/pasted into a different project then the assumption of MainActor isolation may be broken.
The actor worked perfectly well here and was already Swift 6 modern concurrency safe.
| self.completionBlock?([.success(self.choices[index])]) | ||
| self.dismiss(self) | ||
| } | ||
| guard let index = proposedSelectionIndexes.first else { return proposedSelectionIndexes } |
There was a problem hiding this comment.
Previously this code to dismiss the OpenViewController was performed AFTER the proposedSelectionIndexes were returned. With this change, the controller will be dismissed BEFORE the return.
This is probably wrong and the dispatch to run some code AFTER the return should stay here with a comment explaining what it does.
| DispatchQueue.main.async { | ||
| self.saveTo(url: panel.url!) | ||
| } | ||
| self.saveTo(url: panel.url!) |
There was a problem hiding this comment.
This change should also be thoroughly tested because the previous comment talked a bit about why it was dispatched to after the panel closed. With everything on the main thread, this may be fine...or not.
| identitiesPopUpAC.add(contentsOf: identities) | ||
| } catch { | ||
| logger.error("Error loading identities: \(error)") | ||
| Task { |
There was a problem hiding this comment.
Putting this in a Task means that the identitiesPopUpAC will not be filled in with the identities immediately when the view is shown, and will populate later. There is no indication to the user that this loading is happening so if they may try to select an identity, see none, and then not know that there are some identities later.
I recommend adding a small indeterminate progress indicator near the identities popup for the user to see initially, and then at the end of this task the progress indicator can be hidden.
| switch result { | ||
| case .success(let executable): | ||
| guard self?.shouldExecutableBeAdded(executable) ?? false else { | ||
| Task { |
There was a problem hiding this comment.
Putting this in a Task means that the panel will be closed before this Task runs to completion. This could leave the user in a strange state or result in odd bugs if the user tries to do something else in the app before this Task is complete.
| guard let urls = urls, !urls.isEmpty else { return false } | ||
|
|
||
| Task { | ||
| for url in urls { | ||
| do { | ||
| let newExecutable = try await model.loadExecutable(url: url) | ||
| if tableView == self.executablesTable { | ||
| guard self.executablesAC.canInsert else { | ||
| continue | ||
| } | ||
| if self?.shouldExecutableBeAdded(newExecutable) ?? false { | ||
| self?.executablesAC.insert(newExecutable, atArrangedObjectIndex: row) | ||
| addedAny = true | ||
| if self.shouldExecutableBeAdded(newExecutable) { | ||
| self.executablesAC.insert(newExecutable, atArrangedObjectIndex: row) | ||
| } | ||
| } else { | ||
| self?.insertIntoAppleEvents(newExecutable) | ||
| addedAny = true | ||
| self.insertIntoAppleEvents(newExecutable) | ||
| } | ||
| } catch { | ||
| if let loadError = error as? LoadExecutableError { | ||
| self.showAlert(loadError, for: window) | ||
| } | ||
| case .failure(let error): | ||
| self?.showAlert(error, for: window) | ||
| self?.logger.error("\(error)") | ||
| self.logger.error("\(error)") | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return addedAny | ||
| return true | ||
| } |
There was a problem hiding this comment.
Copilot's comment here is correct. Previous behavior checked all of the things synchronously to know if the drop can be accepted. Putting it in a Task moves most of that to after the table already has accepted the drop.
| if let value = current { | ||
| choices = Model.shared.getAppleEventChoices(executable: value) | ||
| Task { | ||
| choices = await Model.shared.getAppleEventChoices(executable: value) |
There was a problem hiding this comment.
This is another case where moving the choices calculation into a Task will mean that the OpenViewController will not have correct choices available to the user upon first opening. They will populate later when this Task is completed.
If desired, an indeterminate progress indicator can be added to the OpenViewController that displays by default and then is hidden at the end of this Task (after choices is set).
Trying to get to main actor isolation for the app.