FMF support#1855
Conversation
![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 introduces integration with Apple's Foundation Models (Apple Intelligence) in the Firebase AI Example app, adding a new "Apple Intelligence" feature screen, a custom local places tool with Google Maps grounding, and a view model to manage hybrid summarization, planning, and visual identification. The review feedback highlights several critical improvements: correcting the iOS availability annotations and checks from future versions (iOS 26.0/27.0) to iOS 18.2, using array indices instead of optional properties as identifiers in SwiftUI ForEach loops to prevent rendering bugs, and refactoring the custom tool into a Sendable struct to ensure concurrency safety.
| import FoundationModels | ||
| import FirebaseAILogic | ||
|
|
||
| @available(iOS 27.0, *) |
There was a problem hiding this comment.
The availability annotation uses iOS 27.0. Please update this to iOS 18.2 to match the actual OS version where Apple Intelligence and FoundationModels are available.
| @available(iOS 27.0, *) | |
| @available(iOS 18.2, *) |
|
|
||
| // Day Plans | ||
| if let days = itinerary.days { | ||
| ForEach(days, id: \.title) { day in |
There was a problem hiding this comment.
Using an optional property like title (which is String? in DayPlan.PartiallyGenerated) as the id for ForEach is unsafe. During streaming or partial generation, multiple elements might have nil or duplicate titles, leading to rendering bugs or crashes in SwiftUI. Using days.indices with id: \.self is a safer approach.
ForEach(days.indices, id: \.self) { index in
let day = days[index]| .padding(.top, 4) | ||
|
|
||
| if let activities = day.activities { | ||
| ForEach(activities, id: \.title) { activity in |
There was a problem hiding this comment.
Using the optional title property as the id for ForEach can cause duplicate identifier issues during partial generation. Using activities.indices with id: \.self is safer.
ForEach(activities.indices, id: \.self) { index in
let activity = activities[index]| Text("Sources & Maps Links") | ||
| .font(.headline) | ||
|
|
||
| ForEach(attributions, id: \.url) { attribution in |
There was a problem hiding this comment.
Using the optional url property as the id for ForEach can cause rendering issues if multiple attributions have nil or duplicate URLs. Using attributions.indices with id: \.self is safer.
ForEach(attributions.indices, id: \.self) { index in
let attribution = attributions[index]| } | ||
|
|
||
| @available(iOS 26.0, *) | ||
| public final class FindLocalPlacesTool { |
There was a problem hiding this comment.
Since FindLocalPlacesTool has no mutable state, declaring it as a struct conforming to Sendable is more idiomatic in Swift and safer for concurrent execution. This also avoids the need for @unchecked Sendable on its wrapper.
| public final class FindLocalPlacesTool { | |
| public struct FindLocalPlacesTool: Sendable { |
|
|
||
| // Struct wrapper to conform to FoundationModels.Tool AND ToolRepresentable | ||
| @available(iOS 26.0, *) | ||
| public struct FindLocalPlacesToolWrapper: FoundationModels.Tool, ToolRepresentable, @unchecked Sendable { |
There was a problem hiding this comment.
If FindLocalPlacesTool is updated to be a Sendable struct, the wrapper can conform to Sendable directly without needing @unchecked Sendable.
| public struct FindLocalPlacesToolWrapper: FoundationModels.Tool, ToolRepresentable, @unchecked Sendable { | |
| public struct FindLocalPlacesToolWrapper: FoundationModels.Tool, ToolRepresentable, Sendable { |
1 participant
No description provided.