Conversation
Summary of ChangesHello @devmike01, 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 resolves an issue where snackbar messages, intended to provide one-time feedback for task operations, would repeatedly appear upon navigation. The solution introduces a robust 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
Activity
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 introduces a solid fix for a bug where snackbars were displayed multiple times. The new approach using a persistent, read-once message queue (AppMessageQueue) is a robust solution. The changes are well-supported by a new integration test that validates the fix. However, I've identified a few issues that should be addressed, including a functional bug causing incorrect snackbar messages, some code duplication, a potential performance issue with SharedPreferences, and a couple of areas for improvement in code clarity and test robustness.
app/src/main/java/com/example/android/architecture/blueprints/todoapp/TodoNavGraph.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/example/android/architecture/blueprints/todoapp/TodoNavigation.kt
Outdated
Show resolved
Hide resolved
...in/java/com/example/android/architecture/blueprints/todoapp/addedittask/AddEditTaskScreen.kt
Outdated
Show resolved
Hide resolved
...ndroidTest/java/com/example/android/architecture/blueprints/todoapp/tasks/TasksScreenTest.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/example/android/architecture/blueprints/todoapp/tasks/TasksScreen.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/example/android/architecture/blueprints/todoapp/util/AppMessageQueue.kt
Outdated
Show resolved
Hide resolved
|
这是来自QQ邮箱的假期自动回复邮件。
您好，我最近正在休假中，无法亲自回复您的邮件。我将在假期结束后，尽快给您回复。
|
|
@gemini-code-assist /gemini review |
There was a problem hiding this comment.
Code Review
This pull request effectively resolves the issue of snackbars appearing multiple times by implementing a SharedPreferences-backed message queue. This ensures that user messages are consumed only once, even across navigation changes. The refactoring of the navigation logic to support this and the addition of a new UI test to verify the fix are excellent improvements. I've included a few suggestions to enhance performance, clean up test code, and remove some now-obsolete code.
| } | ||
|
|
||
| override fun produce(message: Int) { | ||
| prefs.edit(commit = true){ putInt(MESSAGE_KEY, message) } |
There was a problem hiding this comment.
Using commit = true with SharedPreferences.edit() performs a synchronous write to disk, which can block the main thread and cause UI jank. Since this produce method is called from a LaunchedEffect, it runs on the main thread. It's better to use apply() for an asynchronous write. The androidx.core.content.edit extension function uses apply() by default (when commit is false).
| prefs.edit(commit = true){ putInt(MESSAGE_KEY, message) } | |
| prefs.edit { putInt(MESSAGE_KEY, message) } |
| .onNodeWithContentDescription("Save task") | ||
| .performClick() | ||
|
|
||
| snackBar.isDisplayed() |
There was a problem hiding this comment.
The isDisplayed() method returns a boolean indicating if the node is displayed, but it doesn't perform any assertion. This line has no effect on the test result. To verify that the snackbar is displayed, you should use assertIsDisplayed().
| snackBar.isDisplayed() | |
| snackBar.assertIsDisplayed() |
| class TaskTestViewModelFactory(private val repository: TaskRepository) | ||
| : ViewModelProvider.Factory { | ||
| override fun <T : ViewModel> create(modelClass: Class<T>): T { | ||
| return TasksViewModel(repository, SavedStateHandle()) as T | ||
| } | ||
| } | ||
|
|
||
| class AddEditTaskViewModelFactory(private val repository: TaskRepository): ViewModelProvider.Factory { | ||
| override fun <T : ViewModel> create(modelClass: Class<T>): T { | ||
| return AddEditTaskViewModel(repository, SavedStateHandle()) as T | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
These ViewModelProvider.Factory classes (TaskTestViewModelFactory and AddEditTaskViewModelFactory) appear to be unused in the test file. Since Hilt is used for dependency injection, these factories are not needed and can be removed to keep the code clean.
| AppModalDrawer(drawerState, currentRoute, navActions) { | ||
| TasksScreen( | ||
| userMessage = entry.arguments?.getInt(USER_MESSAGE_ARG)!!, | ||
| onUserMessageDisplayed = { entry.arguments?.putInt(USER_MESSAGE_ARG, 0) }, |
There was a problem hiding this comment.
The onUserMessageDisplayed callback seems to be dead code now. It attempts to modify USER_MESSAGE_ARG in the navigation entry's arguments, but this argument is no longer part of the tasks route since the navigation logic was updated. This callback, its parameter in TasksScreen, and its call inside TasksScreen can be removed for better code clarity and maintainability.
|
|
||
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
There are excessive blank lines here, which affect code readability. It's good practice to remove them.
|
@gemini-code-assist /gemini review |
There was a problem hiding this comment.
Code Review
This pull request successfully fixes the issue of snackbars appearing multiple times by refactoring how user messages are passed between screens. Instead of using navigation arguments, it introduces an AppMessageQueue backed by SharedPreferences to handle one-shot messages. The changes are well-tested with a new integration test that verifies the fix.
However, I've found a couple of issues. There's a regression in the navigation logic that affects state restoration when using the navigation drawer. Additionally, there's a minor performance concern in the new AppMessageQueue implementation due to synchronous disk I/O on the main thread. My detailed comments provide suggestions to address these points.
| fun navigateToTasks() { | ||
| navController.navigate( | ||
| TASKS_SCREEN.let { | ||
| if (userMessage != 0) "$it?$USER_MESSAGE_ARG=$userMessage" else it | ||
| } | ||
| TASKS_SCREEN | ||
| ) { | ||
| popUpTo(navController.graph.findStartDestination().id) { | ||
| inclusive = !navigatesFromDrawer | ||
| saveState = navigatesFromDrawer | ||
| inclusive = true | ||
| saveState = false | ||
| } | ||
| launchSingleTop = true | ||
| restoreState = navigatesFromDrawer | ||
| restoreState = false | ||
| } | ||
| } |
There was a problem hiding this comment.
This refactoring of navigateToTasks fixes the snackbar issue but introduces a regression. The TasksScreen state (like scroll position and filter) will no longer be saved and restored when navigating from the drawer, because saveState and restoreState are now always false.
The previous implementation correctly handled two cases: navigating from the drawer (preserving state) and navigating after an action like adding/editing a task (clearing state).
To fix this, you should have two distinct navigation actions: one for navigating from the drawer (which should save state) and one for navigating after an action (which should clear the back stack as you've done here).
The logic for drawer navigation should be similar to navigateToStatistics, which correctly uses saveState = true and restoreState = true.
| } | ||
|
|
||
| override fun produce(message: Int) { | ||
| prefs.edit(commit = true){ putInt(MESSAGE_KEY, message) } |
There was a problem hiding this comment.
Using commit = true with SharedPreferences.edit() performs a synchronous write to disk, which can block the main thread and potentially cause Application Not Responding (ANR) errors. Since this is called from a LaunchedEffect, it runs on the main thread.
It's recommended to use apply() for asynchronous writes. The edit KTX function uses apply() by default. You can simply remove commit = true.
| prefs.edit(commit = true){ putInt(MESSAGE_KEY, message) } | |
| prefs.edit { putInt(MESSAGE_KEY, message) } |
|
@gemini-code-assist /gemini review |
There was a problem hiding this comment.
Code Review
The pull request effectively addresses the issue of snackbars displaying multiple times by introducing an AppMessageQueue mechanism. This is a robust solution for managing transient UI messages across navigation. The changes are well-integrated into the existing architecture, updating navigation logic, ViewModel interactions, and including new test cases to verify the fix. Overall, this is a good improvement to the application's message handling.
| LaunchedEffect(snackbarHostState, viewModel, message, snackbarText) { | ||
| snackbarHostState.showSnackbar(snackbarText) | ||
| viewModel.snackbarMessageShown() | ||
| // viewModel.snackbarMessageShown() |
There was a problem hiding this comment.
The viewModel.snackbarMessageShown() call is commented out here. This means that once a snackbar message is displayed, the ViewModel's internal state for userMessage will not be cleared. This could lead to the same message being re-displayed unexpectedly if the screen recomposes or if the user navigates away and back.
| // viewModel.snackbarMessageShown() | |
| viewModel.snackbarMessageShown() |
|
|
||
|
|
||
| @Composable | ||
| fun TestNavHost(navController: NavHostController, repository: TaskRepository){ |
There was a problem hiding this comment.
The repository parameter in TestNavHost is not used. It's good practice to remove unused parameters to keep the code clean and avoid confusion.
| fun TestNavHost(navController: NavHostController, repository: TaskRepository){ | |
| fun TestNavHost(navController: NavHostController){ |
|
|
||
| package com.example.android.architecture.blueprints.todoapp | ||
|
|
||
| import androidx.compose.ui.window.Popup |
There was a problem hiding this comment.
The import for androidx.compose.ui.window.Popup is not used in this file. Please remove unused imports to maintain code cleanliness.
| } | ||
|
|
||
| fun navigateToStatistics() { | ||
| fun navigateToStatistics() { |
There was a problem hiding this comment.
There's an extra tab indentation for the navigateToStatistics function declaration. Please align it with the class definition.
| fun navigateToStatistics() { | |
| fun navigateToStatistics() { |
| import com.example.android.architecture.blueprints.todoapp.util.Async | ||
| import com.example.android.architecture.blueprints.todoapp.util.WhileUiSubscribed | ||
| import dagger.hilt.android.lifecycle.HiltViewModel | ||
| import kotlinx.coroutines.delay |
There was a problem hiding this comment.
The import kotlinx.coroutines.delay is not used in this file. Please remove unused imports.
| coroutineScope: CoroutineScope = rememberCoroutineScope(), | ||
| content: @Composable () -> Unit | ||
| ) { | ||
| val context = LocalContext.current |
There was a problem hiding this comment.
The context variable is declared but not used within the AppModalDrawer composable. Please remove it if it's not needed.
| composeTestRule.waitForIdle() | ||
|
|
||
| val newTaskBtn = composeTestRule.onNodeWithTag("New Task") | ||
| val snackBar = composeTestRule.onNodeWithText("Task added") |
There was a problem hiding this comment.
The string "Task added" is hardcoded. For better maintainability and localization, consider using a string resource.
|
|
||
| composeTestRule.waitForIdle() | ||
|
|
||
| composeTestRule.onNodeWithTag("Title").performTextInput("TITLE1") |
There was a problem hiding this comment.
The string "TITLE1" is hardcoded. For better maintainability and localization, consider using a string resource.
| composeTestRule.waitForIdle() | ||
|
|
||
| composeTestRule.onNodeWithTag("Title").performTextInput("TITLE1") | ||
| composeTestRule.onNodeWithTag("Enter your task here.").performTextInput("DESCRIPTION1") |
There was a problem hiding this comment.
The string "DESCRIPTION1" is hardcoded. For better maintainability and localization, consider using a string resource.
| composeTestRule.onNodeWithContentDescription("Back") | ||
| .assertIsDisplayed() | ||
| .performClick() | ||
| composeTestRule.onNodeWithTag("Todo") |
There was a problem hiding this comment.
The string "Todo" is hardcoded. For better maintainability and localization, consider using a string resource.
|
@gemini-code-assist /gemini review |
There was a problem hiding this comment.
Code Review
This pull request effectively addresses the issue of duplicate snackbar messages by introducing an AppMessageQueue to manage one-time messages across navigation events. This is a robust solution for handling UI feedback after asynchronous operations and navigation. The addition of testTag modifiers in several composables is also a good practice for improving testability, and the updated tests confirm the new behavior.
app/src/main/java/com/example/android/architecture/blueprints/todoapp/TodoNavigation.kt
Outdated
Show resolved
Hide resolved
|
@gemini-code-assist /gemini review |
There was a problem hiding this comment.
Code Review
This pull request effectively resolves the issue of snackbars appearing multiple times by introducing a message queue system (AppMessageQueue) to handle one-off user messages. This is a great improvement over passing messages via navigation arguments. The changes are well-implemented, and the inclusion of the snackBarOnlyDisplayOnce integration test is excellent for verifying the fix and preventing future regressions. My review includes a few suggestions to further improve code clarity and maintainability, such as using nullable types for the message queue, ensuring consistency in tests by using string resources, and extracting hardcoded strings into constants.
...ndroidTest/java/com/example/android/architecture/blueprints/todoapp/tasks/TasksScreenTest.kt
Outdated
Show resolved
Hide resolved
...ndroidTest/java/com/example/android/architecture/blueprints/todoapp/tasks/TasksScreenTest.kt
Show resolved
Hide resolved
...ndroidTest/java/com/example/android/architecture/blueprints/todoapp/tasks/TasksScreenTest.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/example/android/architecture/blueprints/todoapp/TodoApplication.kt
Show resolved
Hide resolved
app/src/main/java/com/example/android/architecture/blueprints/todoapp/di/DataModules.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/example/android/architecture/blueprints/todoapp/util/AppMessageQueue.kt
Show resolved
Hide resolved
...rc/main/java/com/example/android/architecture/blueprints/todoapp/data/FakeAppMessageQueue.kt
Show resolved
Hide resolved
2 participants
Fixes to creating, adding or editing a task shows the snackbar more than once when navigating.