[WIP] Getting FMF to work#1860
Conversation
Wiz Scan Summary
To detect these findings earlier in the dev lifecycle, try using Wiz Code VS Code Extension. |
There was a problem hiding this comment.
Code Review
This pull request introduces a new "Apple Foundation Models" feature to the Firebase AI Example app, demonstrating integration between Firebase AI Logic (Gemini) and Apple's Foundation Models framework (Apple Intelligence). It includes a hybrid summarization view, a smart planner with local tool calling, and a visual object identifier. Feedback on the implementation focuses on correcting future iOS version availability annotations (updating iOS 26/27 to iOS 18), resolving unsafe SwiftUI ForEach identifiers that use optional properties during streaming, and addressing Swift concurrency anti-patterns where async methods immediately spawn unstructured Tasks with strong self captures.
| import FoundationModels | ||
| import FirebaseAILogic | ||
|
|
||
| @available(iOS 27.0, *) |
| import FoundationModels | ||
| import FirebaseAILogic | ||
|
|
||
| @available(iOS 27.0, *) |
| import FoundationModels | ||
| import FirebaseAILogic | ||
|
|
||
| @available(iOS 26.0, *) |
| if #available(iOS 27.0, *) { | ||
| AppleAIScreen() | ||
| } else { | ||
| Text("Apple Intelligence is only available on iOS 27 or newer.") | ||
| } |
There was a problem hiding this comment.
The availability check is set to iOS 27.0, which is a future version and will cause this feature to always fall back to the else block on current devices (iOS 18). Since Apple Intelligence and the FoundationModels framework are available starting in iOS 18.0/18.2, please update this check to the correct version (e.g., iOS 18.0 or iOS 18.2) so the screen can be accessed and tested.
| if #available(iOS 27.0, *) { | |
| AppleAIScreen() | |
| } else { | |
| Text("Apple Intelligence is only available on iOS 27 or newer.") | |
| } | |
| if #available(iOS 18.0, *) { | |
| AppleAIScreen() | |
| } else { | |
| Text("Apple Intelligence is only available on iOS 18 or newer.") | |
| } |
|
|
||
| // Day Plans | ||
| if let days = itinerary.days { | ||
| ForEach(days, id: \.title) { day in |
There was a problem hiding this comment.
Using optional properties like title as the id in ForEach is unsafe, especially during streaming (where properties are initially nil and then updated character-by-character). When multiple elements have nil for their ID, SwiftUI will encounter duplicate ID conflicts, leading to rendering bugs or crashes. Furthermore, as the title is streamed, its value changes, causing SwiftUI to completely recreate the view for every character. Using Array(enumerated()) with \.offset provides a stable, non-optional ID that prevents these issues.
| ForEach(days, id: \.title) { day in | |
| ForEach(Array(days.enumerated()), id: \.offset) { index, day in |
| public func runSummarization() async { | ||
| stopActiveTask() | ||
| inProgress = true | ||
| error = nil | ||
| outputSummary = nil | ||
|
|
||
| activeTask = Task { | ||
| defer { self.inProgress = false } |
There was a problem hiding this comment.
This function is marked as async but immediately spawns an unstructured Task and returns, which is a concurrency anti-pattern because the async and await keywords become meaningless (they don't actually await the work). Making this function synchronous makes it clear that it starts an asynchronous operation in the background. Additionally, capturing self strongly inside unstructured Tasks can cause temporary memory leaks (preventing the ViewModel from being deallocated when the View is dismissed). Capturing self weakly prevents this.
| public func runSummarization() async { | |
| stopActiveTask() | |
| inProgress = true | |
| error = nil | |
| outputSummary = nil | |
| activeTask = Task { | |
| defer { self.inProgress = false } | |
| public func runSummarization() { | |
| stopActiveTask() | |
| inProgress = true | |
| error = nil | |
| outputSummary = nil | |
| activeTask = Task { [weak self] in | |
| guard let self = self else { return } | |
| defer { self.inProgress = false } |
| public func generateItinerary() async { | ||
| stopActiveTask() | ||
| inProgress = true | ||
| error = nil | ||
| itinerary = nil | ||
|
|
||
| activeTask = Task { | ||
| defer { self.inProgress = false } |
There was a problem hiding this comment.
This function is marked as async but immediately spawns an unstructured Task and returns, which is a concurrency anti-pattern because the async and await keywords become meaningless (they don't actually await the work). Making this function synchronous makes it clear that it starts an asynchronous operation in the background. Additionally, capturing self strongly inside unstructured Tasks can cause temporary memory leaks (preventing the ViewModel from being deallocated when the View is dismissed). Capturing self weakly prevents this.
| public func generateItinerary() async { | |
| stopActiveTask() | |
| inProgress = true | |
| error = nil | |
| itinerary = nil | |
| activeTask = Task { | |
| defer { self.inProgress = false } | |
| public func generateItinerary() { | |
| stopActiveTask() | |
| inProgress = true | |
| error = nil | |
| itinerary = nil | |
| activeTask = Task { [weak self] in | |
| guard let self = self else { return } | |
| defer { self.inProgress = false } |
| public func identifySelectedImage() async { | ||
| guard let image = selectedImage else { return } | ||
| stopActiveTask() | ||
| inProgress = true | ||
| error = nil | ||
| identifiedObject = nil | ||
|
|
||
| activeTask = Task { | ||
| defer { self.inProgress = false } |
There was a problem hiding this comment.
This function is marked as async but immediately spawns an unstructured Task and returns, which is a concurrency anti-pattern because the async and await keywords become meaningless (they don't actually await the work). Making this function synchronous makes it clear that it starts an asynchronous operation in the background. Additionally, capturing self strongly inside unstructured Tasks can cause temporary memory leaks (preventing the ViewModel from being deallocated when the View is dismissed). Capturing self weakly prevents this.
| public func identifySelectedImage() async { | |
| guard let image = selectedImage else { return } | |
| stopActiveTask() | |
| inProgress = true | |
| error = nil | |
| identifiedObject = nil | |
| activeTask = Task { | |
| defer { self.inProgress = false } | |
| public func identifySelectedImage() { | |
| guard let image = selectedImage else { return } | |
| stopActiveTask() | |
| inProgress = true | |
| error = nil | |
| identifiedObject = nil | |
| activeTask = Task { [weak self] in | |
| guard let self = self else { return } | |
| defer { self.inProgress = false } |
| .onChange(of: photosPickerItem) { oldItem, newItem in | ||
| Task { | ||
| if let data = try? await newItem?.loadTransferable(type: Data.self), | ||
| let image = UIImage(data: data) { | ||
| viewModel.selectedImage = image | ||
| await viewModel.identifySelectedImage() | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Since identifySelectedImage() is synchronous, you don't need to await it here.
| .onChange(of: photosPickerItem) { oldItem, newItem in | |
| Task { | |
| if let data = try? await newItem?.loadTransferable(type: Data.self), | |
| let image = UIImage(data: data) { | |
| viewModel.selectedImage = image | |
| await viewModel.identifySelectedImage() | |
| } | |
| } | |
| } | |
| .onChange(of: photosPickerItem) { oldItem, newItem in | |
| Task { | |
| if let data = try? await newItem?.loadTransferable(type: Data.self), | |
| let image = UIImage(data: data) { | |
| viewModel.selectedImage = image | |
| viewModel.identifySelectedImage() | |
| } | |
| } | |
| } |
| Button(action: { | ||
| Task { | ||
| await viewModel.runSummarization() | ||
| } | ||
| }) { |
Replace ConversationKit, enable Vertex AI, no local mode