Conversation
Kteja03
commented
Mar 5, 2026
- Added MainViewModel to manage UI logic
- Added MeshUiState for centralized UI state handling
- Updated MainActivity to observe ViewModel state
- Added MainViewModel with StateFlow - Moved logic from MainActivity into ViewModel - Updated Activity to observe ViewModel state
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 application's main user interface by implementing the MVVM architectural pattern. The primary goal is to decouple the UI from its logic, making the codebase more modular, testable, and easier to maintain. This is achieved by introducing a dedicated ViewModel to manage UI state and handle user interactions, centralizing all UI-related data in a single state object, and transforming the MainActivity into a reactive observer of this state. 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
|
There was a problem hiding this comment.
Code Review
This pull request effectively refactors the MainActivity to use the MVVM pattern by introducing MainViewModel and MeshUiState. This greatly improves the separation of concerns and makes the UI logic more manageable. I've left a few comments with suggestions to further improve consistency with the MVVM pattern and enhance code clarity. Specifically, some logic still remains in the MainActivity that should be moved to the ViewModel, and some methods in the new ViewModel could be made clearer.
| private fun startMeshNetwork() { | ||
| viewModelScope.launch { | ||
| try { | ||
| _uiState.update { it.copy(isLoading = true, statusMessage = "Starting mesh...") } | ||
| // Node starting happens automatically via observeMeshNetwork | ||
| Log.d(TAG, "Mesh network start requested") | ||
| } catch (e: Exception) { | ||
| Log.e(TAG, "Failed to start mesh", e) | ||
| _uiState.update { | ||
| it.copy( | ||
| isLoading = false, | ||
| error = "Failed to start mesh: ${e.message}" | ||
| ) | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The startMeshNetwork method (and stopMeshNetwork as well) is currently misleading. It updates the UI state to indicate loading but does not contain any logic to actually start the network. The comment // Node starting happens automatically via observeMeshNetwork points to an implicit behavior that makes the code harder to understand and maintain.
If an action is required to start the network (e.g., calling a method on the node instance), it should be done within this try block. This would make the method's purpose explicit and the try-catch block would be effective in handling potential errors.
If no action is needed, consider renaming the method and the corresponding UI event to better reflect what they do (e.g., ShowStartingMeshStatus).
There was a problem hiding this comment.
Right now, these methods don’t actually start or stop the mesh node. They just update the UI to show the status, while the real mesh state is handled through observeMeshNetwork(). Since there isn’t a direct start/stop call here yet, I’ve kept the behaviour as is for this PR. I can rename these methods later to make their purpose clearer if needed.
There was a problem hiding this comment.
@Kteja03 Can you just add a comment to this function saying something about this function name being a bit misleading.
|
@Kteja03 Can you address the 3 items Gemini brings up? If you don't think they're legit, just comment under the comment why that is. |
taimuradam
left a comment
There was a problem hiding this comment.
The PR looks good, there are just 2 small changes that could cause incorrect behavior. Removing the RequestPermissionScreen() from MainActivity could cause the app to never ask for wifi/bluetooth permissions and the hasRunBefore() logic in MainViewModel is slightly wrong, since it reads the default values at the start which could cause it to show the wrong screen, for example an onboarding screen to someone that has already been onboarded.
|
I’ve addressed both of the MainActivity comments here. I removed the unused meshPrefs variable, and moved the autoFinish / saveToFolder preference updates to ViewModel events so the Activity only forwards user actions. |
flow733
left a comment
There was a problem hiding this comment.
After going through the changes made, I was only able to dig up the issues already mentioned by Taimur. Since it looks like you just fixed these issues, I have nothing else to comment on. I apologize for the slow turnaround to the pr review; I'm still learning how to do these correctly. Your work appears solid and ready to me.
taimuradam
left a comment
There was a problem hiding this comment.
Looks good to me, the mentioned problems were fixed.