refactor(ai-logic): split samples into various ViewModels#2767
refactor(ai-logic): split samples into various ViewModels#2767thatfiredev wants to merge 24 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
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.
| val city = functionCall.args["city"]!!.jsonPrimitive.content | ||
| val state = functionCall.args["state"]!!.jsonPrimitive.content // Fixed state retrieval | ||
| val date = functionCall.args["date"]!!.jsonPrimitive.content |
There was a problem hiding this comment.
Using the non-null assertion operator (!!) on arguments from the model's function call is risky. If the model's response doesn't include the expected arguments for any reason, the app will crash with a NullPointerException. It's much safer to use safe calls (?.) and gracefully handle cases where arguments might be missing.
| val city = functionCall.args["city"]!!.jsonPrimitive.content | |
| val state = functionCall.args["state"]!!.jsonPrimitive.content // Fixed state retrieval | |
| val date = functionCall.args["date"]!!.jsonPrimitive.content | |
| val city = functionCall.args["city"]?.jsonPrimitive?.content | |
| val state = functionCall.args["state"]?.jsonPrimitive?.content | |
| val date = functionCall.args["date"]?.jsonPrimitive?.content |
| 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)) |
| fun TextGenScreen( | ||
| textGenViewModel: TextGenViewModel = viewModel<TextGenViewModel>() | ||
| fun ServerPromptScreen( | ||
| viewModel: ServerPromptTemplateViewModel = viewModel() |
There was a problem hiding this comment.
The viewModel parameter includes a default viewModel() factory call. Since the ViewModel is now consistently provided from the navigation graph in MainActivity, this default parameter is unnecessary and potentially misleading. Removing it makes the composable's dependency on a parent-provided ViewModel explicit and improves clarity.
| viewModel: ServerPromptTemplateViewModel = viewModel() | |
| viewModel: ServerPromptTemplateViewModel |
|
|
||
| @Composable | ||
| fun SvgScreen( | ||
| svgViewModel: SvgViewModel = viewModel<SvgViewModel>() |
There was a problem hiding this comment.
The svgViewModel parameter has a default value viewModel<SvgViewModel>(). As the ViewModel is now provided from the navigation graph in MainActivity, this default is redundant and can be confusing. It's best practice to remove it to make the dependency explicit.
| svgViewModel: SvgViewModel = viewModel<SvgViewModel>() | |
| svgViewModel: SvgViewModel |
1 participant
The goal is to make these samples more readable by splitting them into various ViewModels - each feature has its own ViewModel.