refactor(ai-logic): split samples into various ViewModels#2767
refactor(ai-logic): split samples into various ViewModels#2767thatfiredev wants to merge 26 commits intomasterfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the sample application's architecture by decomposing its AI features into distinct ViewModel components. This change aims to enhance the modularity and maintainability of the codebase, making it easier for developers to understand and extend individual AI functionalities. The updated navigation system now leverages these new ViewModels, streamlining the process of integrating and showcasing various AI capabilities within the app. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 is a great architectural refactoring, splitting monolithic sample definitions and ViewModels into smaller, feature-specific ones. This significantly improves code organization, readability, and maintainability. The adoption of UiState sealed interfaces for screen state management is another excellent change that aligns with modern Android development best practices. I've identified a few areas for improvement, including a critical issue with the BidiViewModel that appears to break a feature, along with some opportunities to enhance code safety and consistency.
firebase-ai/app/src/main/java/com/google/firebase/quickstart/ai/feature/live/BidiViewModel.kt
Outdated
Show resolved
Hide resolved
...-ai/app/src/main/java/com/google/firebase/quickstart/ai/feature/text/WeatherChatViewModel.kt
Outdated
Show resolved
Hide resolved
firebase-ai/app/src/main/java/com/google/firebase/quickstart/ai/MainActivity.kt
Outdated
Show resolved
Hide resolved
| val errorMessage by svgViewModel.errorMessage.collectAsStateWithLifecycle() | ||
| val isLoading by svgViewModel.isLoading.collectAsStateWithLifecycle() | ||
| val generatedSvgs by svgViewModel.generatedSvgs.collectAsStateWithLifecycle() | ||
| var prompt by rememberSaveable { mutableStateOf("A kitten") } |
There was a problem hiding this comment.
The initial prompt is hardcoded in the UI layer. For better separation of concerns and consistency with the other refactored screens, this value should be defined in the SvgViewModel and consumed here.
To fix this, you can add val initialPrompt: String = "a kitten" to SvgViewModel and then update this line to use svgViewModel.initialPrompt.
| var prompt by rememberSaveable { mutableStateOf("A kitten") } | |
| var prompt by rememberSaveable { mutableStateOf(svgViewModel.initialPrompt) } |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This is an excellent and extensive refactoring that greatly improves the project's structure and maintainability. Splitting the monolithic sample file into feature-specific ViewModels, introducing abstract base classes for ChatViewModel and ImagenViewModel, and using sealed interfaces for UI state are all fantastic architectural improvements. The code is now much more organized and easier to follow. I've found a few issues, including a critical one related to blocking the main thread, that should be addressed to ensure the app's stability and performance.
| ) | ||
| ), | ||
| ) | ||
| runBlocking { liveSession = liveModel.connect() } |
There was a problem hiding this comment.
Using runBlocking in the init block is a critical issue. The init block of a ViewModel is executed on the main thread, and runBlocking will block this thread while waiting for the network operation (liveModel.connect()) to complete. This will cause the UI to freeze and can lead to an Application Not Responding (ANR) error.
Please perform this operation asynchronously using viewModelScope on a background dispatcher. You will need to manage the connection state (e.g., with a StateFlow) to ensure liveSession is initialized before it's used by functions like startConversation().
A similar issue exists in StreamVideoViewModel.kt.
...-ai/app/src/main/java/com/google/firebase/quickstart/ai/feature/text/WeatherChatViewModel.kt
Outdated
Show resolved
Hide resolved
| currentState: ImagenUiState.Success | ||
| ): ImagenGenerationResponse<ImagenInlineImage> { | ||
| return try { | ||
| templateImagenModel.generateImages("imagen-basic", mapOf("prompt" to inputText)) |
There was a problem hiding this comment.
The template ID "imagen-basic" and key "prompt" are hardcoded as magic strings. This makes the code harder to read and maintain, and increases the risk of typos. It would be better to extract these into private const val at the top of the file or in a companion object.
| templateImagenModel.generateImages("imagen-basic", mapOf("prompt" to inputText)) | |
| templateImagenModel.generateImages(TEMPLATE_ID, mapOf(TEMPLATE_PROMPT_KEY to inputText)) |
| _uiState.value = ServerPromptUiState.Loading | ||
| try { | ||
| val response = templateGenerativeModel | ||
| .generateContent("input-system-instructions", mapOf("customerName" to inputText)) |
There was a problem hiding this comment.
The template ID "input-system-instructions" and key "customerName" are hardcoded. This makes the code harder to read and maintain. Please extract these magic strings into named constants.
| .generateContent("input-system-instructions", mapOf("customerName" to inputText)) | |
| .generateContent(TEMPLATE_ID, mapOf(TEMPLATE_CUSTOMER_NAME_KEY to inputText)) |
firebase-ai/app/src/main/java/com/google/firebase/quickstart/ai/ui/ServerPromptScreen.kt
Outdated
Show resolved
Hide resolved
firebase-ai/app/src/main/java/com/google/firebase/quickstart/ai/ui/SvgScreen.kt
Outdated
Show resolved
Hide resolved
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
rlazo
left a comment
rlazo
left a comment
There was a problem hiding this comment.
Few comments, but nothing blocking .
| tools = listOf( | ||
| Tool.functionDeclarations( | ||
| listOf( | ||
| FunctionDeclaration( |
There was a problem hiding this comment.
IIRC there are a few places were we use the same Tool declaration. Would it make sense to abstract that into its own file and clean up some of the declaration? On the other hand, it would make it harder to copy&paste.
I don't have strong feelings either way.
| } | ||
|
|
||
| override fun handler(functionCall: FunctionCallPart): FunctionResponsePart { | ||
| val response: JsonObject |
There was a problem hiding this comment.
If you decide to abstract function calling, this can be abstracted too.
| } | ||
|
|
||
| val liveModel = | ||
| Firebase.ai(backend = GenerativeBackend.googleAI()) |
There was a problem hiding this comment.
Could you add a comment with the link to the list of supported models by backend? Last time I had to test this it was a pain to remember that names do not align and you need different names for the same model if using different backends
|
|
||
| class ImageGenerationViewModel : ChatViewModel() { | ||
|
|
||
| override val initialPrompt: String = "Hi, can you create a 3d rendered image of a pig " + |
There was a problem hiding this comment.
triple quotes and trimIdent?
|
|
||
| chat = generativeModel.startChat( | ||
| history = listOf( | ||
| content { |
There was a problem hiding this comment.
this can be content("user") {... and content("model") {...
2 participants
The goal is to make these samples more readable by splitting them into various ViewModels - each sample/feature has its own ViewModel.
There should be no UI change (other than typo fixes) or functionality changes.