Refactor SignInViewModel for better state management here

[junior-od]

Refactor SignInViewModel to improve state management and error handling.

[gemini-code-assist]

Summary of Changes

Hello @junior-od, I'm Gemini Code Assist¹! 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 initiates a refactoring effort within the SignInViewModel by explicitly identifying and documenting several areas that require improvement in state management, error handling, and null safety. Instead of implementing the fixes, the changes primarily involve adding detailed comments that highlight existing issues and potential anti-patterns, such as direct mutable state exposure, missing input validation, and the risky use of force unwrapping. This PR serves as a roadmap for subsequent code enhancements.

Highlights

• Identified State Exposure: Added comments to highlight the direct exposure of mutable MutableStateFlow instances (_signInUiState, _email) in SignInViewModel, indicating a potential state management issue.
• Flagged signInUser Issues: Introduced comments to signInUser function, pointing out the unnecessary suspend keyword, nested viewModelScope.launch, and the absence of input validation before execution.
• Highlighted Error Handling Gaps: Added comments to signInUserWithGoogle and saveUser functions, noting the lack of comprehensive error handling for exceptions and database operations, respectively.
• Warned Against Force Unwrapping: Introduced a critical comment in signInUserWithGoogle to explicitly flag the use of force unwrapping (!!) for user properties, which could lead to NullPointerExceptions.
• Removed Redundant Comments: Cleaned up the codebase by removing verbose Javadoc-style comments from simple update functions (updateEmail, updatePassword, updatePasswordVisible).

Using Gemini Code Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request refactors the SignInViewModel with changes to state management and business logic. While the goal is to improve the ViewModel, several changes introduce significant issues that go against modern Android development best practices. My review focuses on correcting these issues to ensure the state is managed safely and robustly. Key areas of feedback include:

• State Encapsulation: Several MutableStateFlow properties are exposed publicly and mutably, breaking encapsulation. They should be private with immutable StateFlows exposed.
• Error Handling: Critical functions for signing in and saving user data lack proper exception handling (try-catch), which can lead to app crashes.
• Null Safety: The code uses forced unwrapping (!!) on nullable objects, which is unsafe and can cause NullPointerExceptions.
• Coroutine Usage: There are instances of redundant suspend modifiers and nested coroutine launches that should be simplified.
• Input Validation: The sign-in logic lacks basic input validation.

I've provided specific suggestions to address these points and align the code with recommended patterns for state management and error handling in a ViewModel.

[gemini-code-assist]

Exposing MutableStateFlow as a public var breaks encapsulation. It allows any part of your app to not only change the state directly but also reassign the StateFlow object itself, leading to unpredictable behavior and bugs that are hard to trace. The backing property should be private val. Additionally, the public property signInUiState should expose an immutable StateFlow to prevent external modifications.

Suggested change

	var _signInUiState = MutableStateFlow<SignInUi?>(null)
	private val _signInUiState = MutableStateFlow<SignInUi?>(null)

[gemini-code-assist]

The _email MutableStateFlow is exposed as a public var. This breaks encapsulation and is not a safe way to handle UI state. It should be a private val, and the public email property should expose an immutable StateFlow.

Suggested change

	var _email = MutableStateFlow("")
	private val _email = MutableStateFlow("")

[gemini-code-assist]

Using the not-null assertion operator (!!) is unsafe and can cause a NullPointerException if the user object from Google Sign-In is null, or if its displayName or email properties are null. This will crash your app. You should handle the nullable values safely using safe calls and null checks.

                        user?.let { firebaseUser ->
                            saveUser(
                                userId = firebaseUser.uid,
                                user = User(
                                    id = "",
                                    firstName = firebaseUser.displayName ?: "",
                                    email = firebaseUser.email ?: ""
                                )
                            )
                            _signInUiState.value = SignInUi.Success
                        } ?: run {
                            _signInUiState.value = SignInUi.Error("Failed to retrieve user information.")
                        }

[gemini-code-assist]

The signInUser function is marked as suspend, but it immediately launches a new coroutine on viewModelScope. This is redundant. A ViewModel function that starts a fire-and-forget coroutine should not typically be suspend. The caller in SignInScreen also wraps this in another launch, creating an unnecessary nested coroutine.

Suggested change

	suspend fun signInUser(){
	fun signInUser(){

[gemini-code-assist]

The sign-in process is initiated without validating the email and password. This can lead to unnecessary network requests with invalid data. You should add checks to ensure the fields are not empty before proceeding.

            if (_email.value.isBlank() || _password.value.isBlank()) {
                _signInUiState.value = SignInUi.Error("Email and password cannot be empty.")
                return@launch
            }

            signInUseCase(
                email = _email.value,
                password = _password.value,

[gemini-code-assist]

This call to createUserUseCase performs a database operation without any error handling. If the operation fails (e.g., due to a database constraint), it will throw an exception that will propagate up and crash the coroutine. You should wrap this call in a try-catch block to handle potential exceptions gracefully, at least by logging them.

[gemini-code-assist]

The backing property _password is declared as var. Since the MutableStateFlow instance itself is never reassigned (only its internal value), this should be a val for immutability.

Suggested change

	private var _password = MutableStateFlow("")
	private val _password = MutableStateFlow("")

[gemini-code-assist]

The backing property _passwordVisible is declared as var. It should be a val because the MutableStateFlow object is not being reassigned.

Suggested change

	private var _passwordVisible = MutableStateFlow(false)
	private val _passwordVisible = MutableStateFlow(false)

[gemini-code-assist]

This nested viewModelScope.launch is unnecessary. The onResultCallback is already executing within the coroutine launched by signInUser. You can update the _signInUiState value directly.

                            _signInUiState.value = SignInUi.Success

[gemini-code-assist]

The file is missing a final newline character. It's a standard convention and good practice to end files with a newline. Many tools and editors expect this.