Update travel app to talk directly to Firebase AI Logic rather than use firebase_ai_client to enable easier tool integration #276
Conversation
|
/gemini summarize |
![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 refactors the travel app to directly use the Firebase AI SDK instead of the firebase_ai_client, introducing a new GeminiClient and replacing ChatMessage with a Turn sealed class. The changes are a good step towards a cleaner architecture. However, I've identified a critical issue with testability due to the removal of dependency injection, as well as a couple of high-severity issues related to unsafe list access that could lead to runtime errors. I've also included some medium-severity suggestions for improving code quality and maintainability.
| /// Creates a new [TravelPlannerPage]. | ||
| /// | ||
| /// An optional [aiClient] can be provided, which is useful for testing | ||
| /// or using a custom AI client implementation. If not provided, a default | ||
| /// [FirebaseAiClient] is created. | ||
| const TravelPlannerPage({this.aiClient, super.key}); | ||
|
|
||
| /// The AI client to use for the application. | ||
| /// | ||
| /// If null, a default instance of [FirebaseAiClient] will be created within | ||
| /// the page's state. | ||
| final AiClient? aiClient; | ||
| const TravelPlannerPage({super.key}); |
There was a problem hiding this comment.
The removal of dependency injection for the AI client has made TravelPlannerPage difficult to test and has broken existing unit tests in main_test.dart. The page now creates its own GeminiClient instance directly, preventing the injection of a mock client during tests. This forces tests to either run against the actual Firebase backend (making them integration tests) or fail.
To maintain testability, please consider re-introducing dependency injection. You could pass a GeminiClient instance to the TravelPlannerPage widget, which can be provided from TravelApp. This is crucial for effective unit testing.
| .whereType<Content>() | ||
| .toList(); | ||
| final result = await _geminiClient.generate(contentHistory); | ||
| final value = result.candidates.first.content.parts |
There was a problem hiding this comment.
Accessing result.candidates.first without checking if the list is empty can lead to a StateError if the model returns no candidates (e.g., due to safety filters). This would be caught by the try/finally, but the user would see the thinking indicator disappear without any feedback. It's safer to check if the candidates list is empty before accessing its first element.
|
|
||
| final functionResponses = <fai.FunctionResponse>[]; | ||
| for (final call in functionCalls) { | ||
| final tool = tools.firstWhere((t) => t.name == call.name); |
There was a problem hiding this comment.
Using firstWhere without an orElse clause can lead to a StateError if the AI model hallucinates a function call to a tool that doesn't exist in the tools list. This will interrupt the inference flow and result in a poor user experience as the app will just stop responding without an error.
A safer approach would be to use a method that doesn't throw, like indexWhere followed by a check for -1, or package:collection's firstWhereOrNull. If a tool is not found, you could log a warning and add a FunctionResponse to the history informing the model about the error, allowing it to potentially recover.
| void _configureLogging() { | ||
| hierarchicalLoggingEnabled = true; | ||
| Logger.root.level = Level.ALL; | ||
| Logger.root.onRecord.listen((record) { | ||
| // ignore: avoid_print | ||
| print( | ||
| '[${record.level.name}] ${record.time}: ' | ||
| '${record.loggerName}: ${record.message}', | ||
| ); | ||
| }); | ||
| } |
There was a problem hiding this comment.
The _configureLogging function uses print to output log records. The // ignore: avoid_print indicates you're aware of the lint. For better practice, consider using log from dart:developer (you'll need to import it as e.g. 'developer'). It's automatically stripped from release builds and integrates well with developer tools.
| void _configureLogging() { | |
| hierarchicalLoggingEnabled = true; | |
| Logger.root.level = Level.ALL; | |
| Logger.root.onRecord.listen((record) { | |
| // ignore: avoid_print | |
| print( | |
| '[${record.level.name}] ${record.time}: ' | |
| '${record.loggerName}: ${record.message}', | |
| ); | |
| }); | |
| } | |
| void _configureLogging() { | |
| hierarchicalLoggingEnabled = true; | |
| Logger.root.level = Level.ALL; | |
| Logger.root.onRecord.listen((record) { | |
| developer.log( | |
| record.message, | |
| time: record.time, | |
| level: record.level.value, | |
| name: record.loggerName, | |
| error: record.error, | |
| stackTrace: record.stackTrace, | |
| ); | |
| }); | |
| } |
| @@ -386,6 +368,7 @@ to the user. | |||
| 3. Create an initial itinerary, which will be iterated over in subsequent | |||
| steps. This involves planning out each day of the trip, including the | |||
| specific locations and draft activities. For shorter trips where the | |||
|
|
|||
There was a problem hiding this comment.
An extra newline has been added here, which seems unintentional. While it may not have a significant impact, it's best to keep prompts clean from accidental formatting changes as they can sometimes subtly affect model behavior.
| class UserTurn extends Turn { | ||
| final String text; | ||
|
|
||
| const UserTurn(this.text); | ||
|
|
||
| @override | ||
| firebase_ai.Content toContent() { | ||
| return firebase_ai.Content('user', [firebase_ai.TextPart(text)]); | ||
| } | ||
| } | ||
|
|
||
| class UserUiInteractionTurn extends Turn { | ||
| final String text; | ||
|
|
||
| const UserUiInteractionTurn(this.text); | ||
|
|
||
| @override | ||
| firebase_ai.Content toContent() { | ||
| return firebase_ai.Content('user', [firebase_ai.TextPart(text)]); | ||
| } | ||
| } |
There was a problem hiding this comment.
The classes UserTurn and UserUiInteractionTurn are structurally identical. To avoid code duplication and improve maintainability, consider creating a common abstract base class for them that contains the shared text field and toContent implementation. The distinct concrete classes are still useful for how they are handled differently in the UI.
Example:
abstract class _UserTextTurn extends Turn {
final String text;
const _UserTextTurn(this.text);
@override
firebase_ai.Content toContent() {
return firebase_ai.Content('user', [firebase_ai.TextPart(text)]);
}
}
class UserTurn extends _UserTextTurn {
const UserTurn(super.text);
}
class UserUiInteractionTurn extends _UserTextTurn {
const UserUiInteractionTurn(super.text);
}
Summary of ChangesThis pull request significantly refactors the Highlights
Changelog
Activity
|
This pull request significantly refactors the
travel_app's AI communication layer. The core change involves migrating from thefirebase_ai_clientpackage to a direct integration withFirebase AI Logic, which is intended to simplify and improve the integration of AI tools. This update streamlines the application's interaction with generative AI models and addresses existing issues related to the generative UI framework.Highlights
firebase_ai_clientpackage to directly interacting withFirebase AI Logic.GeminiClientclass has been introduced to manage AI model interactions, including tool declaration adaptation and content generation with tool usage cycles.Turnhierarchy, providing a clearer and more robust way to manage different types of conversational turns.aiClientdependency injection has been removed fromTravelAppandTravelPlannerPage, simplifying their initialization.Changelog
firebase_aidirectly and introduced newGeminiClientandTurntypes.aiClientparameter fromTravelAppandTravelPlannerPageconstructors, simplifying their initialization.GeminiClientand theTurnconversation model._configureLogging)._schemaforTextInputChipto emphasize its usage within anInputGroup.GeminiClientclass, responsible for managing AI model interactions, including tool declaration adaptation and content generation with tool usage cycles.Turnand its subclasses (UserTurn,UserUiInteractionTurn,AiTextTurn,GenUiTurn) to represent different types of conversational turns, replacing the previousChatMessagehierarchy.Turnhierarchy for displaying conversation messages.switchexpression onTurntypes.userPromptBuilderandshowInternalMessagesoptions.aiClientparameter fromTravelApp's constructor.Turntypes (UserTurn,GenUiTurn) for conversation messages.userPromptBuilder.GeminiSchemaAdapterandtools.dart, and removed the export for the movedchat_primitives.dart.ChatMessageWidgetimplementation was moved from theflutter_genuipackage to thetravel_appexample, now located atexamples/travel_app/lib/src/widgets/chat_message.dart.This is intended to help fix #40 and then #134