Authentication improvement#146
Conversation
This commit completely refactors the device token polling mechanism for GitHub authentication to improve its robustness, error handling, and resilience.
Key changes include:
* **Replaced Timeout Logic:** The manual countdown timer (`remainingMs`) is replaced with a `System.currentTimeMillis()` check against a start time, ensuring a more reliable timeout. The loop now runs as long as the coroutine is `isActive`.
* **Improved Error Handling:**
* Introduced distinct counters for network errors and other unknown errors, allowing for more specific retry strategies.
* Added a dedicated `isNetworkError()` function to classify a wider range of network-related exceptions.
* Refined handling for specific API responses like `slow_down`, `access_denied`, `expired_token`, and `bad_verification_code` with clearer user-facing messages.
* **Enhanced Retry and Backoff:**
* Implemented more sophisticated backoff strategies that differentiate between network issues and other failures.
* The `slow_down` response now progressively increases the polling interval.
* **Token Save-and-Verify:**
* Introduced a new `saveTokenWithVerification` function that attempts to save the token and then immediately verifies its presence in the data source, retrying on failure.
* **Code Structure and Readability:** The overall logic is cleaner, with more descriptive variable names and detailed logging to aid in debugging authentication flows.
This commit refactors the `AuthenticationViewModel` to improve coroutine context switching and error handling during the authentication flow. - Switched network and long-running operations to `Dispatchers.IO`. - Ensured UI state updates and navigation events occur on `Dispatchers.Main.immediate`. - Added `try-catch` blocks for browser and clipboard interactions to prevent crashes and log potential failures.
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Fix all issues with AI Agents 🤖
In
@composeApp/src/commonMain/kotlin/zed/rainxch/githubstore/feature/auth/data/repository/AuthenticationRepositoryImpl.kt:
- Around line 237-254: The withRetry function is dead code and also incorrectly
swallows CancellationException; either remove withRetry entirely or integrate it
into saveTokenWithVerification by replacing the direct retry logic with calls to
withRetry, and if you keep withRetry, change its catch to rethrow
CancellationException (e.g., catch Throwable and if it is CancellationException
rethrow, otherwise handle) and ensure callers expect the suspension behavior;
reference the withRetry function and the saveTokenWithVerification method when
making the replacement or removal to locate the right spot.
- Around line 196-220: The retry loop in saveTokenWithVerification currently
catches all Exceptions and will swallow CancellationException, preventing
coroutine cancellation; update the error handling so CancellationException is
not caught—either add a specific catch for CancellationException that rethrows
or check `if (e is CancellationException) throw e` at the top of the existing
catch block—then continue to handle other exceptions as before (when calling
tokenDataSource.save/ tokenDataSource.current).
- Around line 69-73: Replace the thrown CancellationException in
AuthenticationRepositoryImpl (the timeout branch comparing
System.currentTimeMillis() - startTime >= timeoutMs) with a dedicated exception
type for timeouts (e.g., create and throw AuthenticationTimeoutException :
Exception(...) or throw java.util.concurrent.TimeoutException with the same
message); update imports and any callers/handlers if they currently expect
CancellationException so that timeout is treated as an error condition rather
than coroutine cancellation.
- Around line 118-130: The code in AuthenticationRepositoryImpl treats similar
user-actionable OAuth errors inconsistently by throwing CancellationException
for "access_denied" and expired token cases while other actionable errors (e.g.,
"bad_verification_code") throw a regular Exception; update the branches that
currently throw CancellationException to throw a standard Exception with the
same user-facing message so all user-actionable auth errors are handled
consistently (locate the error handling switch/when that inspects errorMsg in
AuthenticationRepositoryImpl and replace CancellationException(...) with
Exception(...)).
In
@composeApp/src/commonMain/kotlin/zed/rainxch/githubstore/feature/auth/presentation/AuthenticationViewModel.kt:
- Around line 122-131: Don't swallow CancellationException in the
AuthenticationViewModel: remove or rethrow it instead of converting it to an
AuthLoginState.Error. In the catch block that currently catches
CancellationException (surrounding the call that uses
withContext(Dispatchers.Main.immediate) and _state.update to set
AuthLoginState.Error with getString(...)), either omit catching
CancellationException entirely or rethrow it (throw e) immediately so coroutine
cancellation propagates; only handle non-cancellation exceptions when updating
_state.update.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
composeApp/src/commonMain/kotlin/zed/rainxch/githubstore/feature/auth/presentation/AuthenticationViewModel.kt (1)
43-59: UseviewModelScopefor the token flow collection to match the ViewModel's lifecycle.The token collection in
onStartuses the injected globalscope, whilestateInand other operations useviewModelScope. Since the injected scope (an app-level singleton withSupervisorJob) is never cancelled, the collection will continue after the ViewModel is destroyed, preventing garbage collection. Replacescope.launchon line 46 withviewModelScope.launch, or if app-level token tracking is intentional, document this and ensure proper lifecycle handling.
🧹 Nitpick comments (3)
composeApp/src/commonMain/kotlin/zed/rainxch/githubstore/feature/auth/data/repository/AuthenticationRepositoryImpl.kt (1)
191-194: Unreachable code afterwhile (isActive)loop.If
isActivebecomesfalse, the coroutine is being cancelled, and the code at line 193 will likely not execute or will be superseded by the cancellation mechanism. This line serves as a defensive fallback but may be unnecessary.composeApp/src/commonMain/kotlin/zed/rainxch/githubstore/feature/auth/presentation/AuthenticationViewModel.kt (2)
146-155: Silent failure when browser fails to open.If
browserHelper.openUrl()fails, the error is only logged. The user receives no feedback that the browser didn't open. Consider updating state or sending an event to notify the user.
90-92: Redundant dispatcher switching.Both
startDeviceFlow()andawaitDeviceToken()already usewithContext(Dispatchers.IO)internally in the repository. Wrapping them again withwithContext(Dispatchers.IO)in the ViewModel is unnecessary overhead.🔎 Proposed simplification
- val start = withContext(Dispatchers.IO) { - authenticationRepository.startDeviceFlow() - } + val start = authenticationRepository.startDeviceFlow() ... - withContext(Dispatchers.IO) { - authenticationRepository.awaitDeviceToken(start = start) - } + authenticationRepository.awaitDeviceToken(start = start)Also applies to: 113-115
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
composeApp/src/commonMain/kotlin/zed/rainxch/githubstore/feature/auth/data/repository/AuthenticationRepositoryImpl.ktcomposeApp/src/commonMain/kotlin/zed/rainxch/githubstore/feature/auth/presentation/AuthenticationViewModel.kt
🔇 Additional comments (3)
composeApp/src/commonMain/kotlin/zed/rainxch/githubstore/feature/auth/data/repository/AuthenticationRepositoryImpl.kt (2)
222-234: LGTM - Network error classification.The string-based heuristic covers common network error patterns. This is a reasonable approach for error classification in the context of retry decisions.
56-56: No action needed.System.currentTimeMillis()is fully supported on this project's Kotlin Multiplatform targets (Android and JVM). The code works correctly for the configured platforms.Likely an incorrect or invalid review comment.
composeApp/src/commonMain/kotlin/zed/rainxch/githubstore/feature/auth/presentation/AuthenticationViewModel.kt (1)
94-111: LGTM - Clipboard handling in startLogin.The state is correctly set with
copied = falsefirst, then only updated totrueafter successful copy. This is the correct pattern, unlike thecopyCodefunction.
This commit refactors the authentication flow with several key improvements:
- **Replaced `CancellationException` with more specific exceptions:**
- `TimeoutException` is now thrown when the authentication process times out.
- Generic `Exception` is used for errors like "access denied" or "expired token" to provide clearer error messages to the user.
- **Improved exception propagation:** Unhandled `CancellationException` and `TimeoutException` are now correctly re-thrown in the polling and token-saving logic, preventing them from being caught by generic `catch` blocks.
- **Removed `withRetry` utility:** The redundant `withRetry` helper function has been deleted as retry logic is already handled within the authentication flow.
- **Refined `copyCode` logic:** In the `AuthenticationViewModel`, the clipboard copy operation is now performed before updating the UI state to ensure a more consistent user experience.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
composeApp/src/commonMain/kotlin/zed/rainxch/githubstore/feature/auth/presentation/AuthenticationViewModel.kt (1)
101-109: Consider surfacing clipboard failures to the user.When clipboard copy fails, only the
copiedflag remains false without any visible error feedback. Users might not understand why the code wasn't copied automatically.🔎 Optional enhancement - add subtle error indicator
Consider adding a transient info message:
try { clipboardHelper.copy( label = getString(Res.string.enter_code_on_github), text = start.userCode ) _state.update { it.copy(copied = true) } } catch (e: Exception) { Logger.d { "⚠️ Failed to copy to clipboard: ${e.message}" } + _state.update { + it.copy( + copied = false, + info = "Could not copy code automatically. Please copy manually." + ) + } }composeApp/src/commonMain/kotlin/zed/rainxch/githubstore/feature/auth/data/repository/AuthenticationRepositoryImpl.kt (3)
105-120: Consider capping the polling interval growth.The
slow_downhandler increasespollingIntervalby 5 seconds on each occurrence without an upper bound. After 10 consecutive slow-downs, the interval could reachinitialInterval + 50s, which might be excessive.🔎 Optional improvement - cap polling interval
"slow_down" in errorMsg -> { consecutiveNetworkErrors = 0 consecutiveUnknownErrors = 0 slowDownCount++ - pollingInterval += 5000 + pollingInterval = minOf(pollingInterval + 5000, 60_000L) Logger.d { "⚠️ Rate limited. New interval: ${pollingInterval}ms (slowdown #$slowDownCount)" } if (slowDownCount > 10) { throw Exception( "GitHub is experiencing high traffic. Please wait a few minutes and try again." ) } delay(pollingInterval + (0..3000).random()) }
184-196: Network exceptions may bypass network-specific handling.The catch block treats all exceptions as unknown errors and increments
consecutiveUnknownErrors. However, network-related exceptions (e.g.,UnknownHostException,SocketTimeoutException) thrown during the API call itself won't benefit from the network-specific backoff strategy (lines 143-160) sinceisNetworkError()only inspects error messages from API responses.🔎 Optional enhancement - classify exception types
Consider extending
isNetworkErrorto also accept exception types:private fun isNetworkError(errorMsg: String): Boolean { return errorMsg.contains("unable to resolve") || // ... existing checks } private fun isNetworkError(exception: Exception): Boolean { return exception is java.net.UnknownHostException || exception is java.net.SocketTimeoutException || exception is java.net.ConnectException || exception.message?.let { isNetworkError(it.lowercase()) } == true }Then update the catch block:
} catch (e: Exception) { - consecutiveUnknownErrors++ - Logger.d { "❌ Unexpected error ($consecutiveUnknownErrors/5): ${e.message}" } + if (isNetworkError(e)) { + consecutiveNetworkErrors++ + // Use network backoff... + } else { + consecutiveUnknownErrors++ + Logger.d { "❌ Unexpected error ($consecutiveUnknownErrors/5): ${e.message}" } // ... rest of unknown error handling + }
202-228: Error message could be more precise.The exception message at line 223 states "Failed to save authentication token" but the failure could occur during either the save operation (line 205) or the verification read (line 208).
🔎 Optional improvement - clarify error source
} catch (e: Exception) { Logger.d { "⚠️ Token save failed (attempt ${attempt + 1}/5): ${e.message}" } if (attempt == 4) { - throw Exception("Failed to save authentication token: ${e.message}", e) + throw Exception("Failed to save or verify authentication token: ${e.message}", e) } delay(500L * (attempt + 1)) }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
composeApp/src/commonMain/kotlin/zed/rainxch/githubstore/feature/auth/data/repository/AuthenticationRepositoryImpl.ktcomposeApp/src/commonMain/kotlin/zed/rainxch/githubstore/feature/auth/presentation/AuthenticationViewModel.kt
🔇 Additional comments (6)
composeApp/src/commonMain/kotlin/zed/rainxch/githubstore/feature/auth/presentation/AuthenticationViewModel.kt (2)
121-122: CancellationException now properly rethrown.The previous issue with swallowing
CancellationExceptionhas been correctly resolved. The exception is now rethrown to allow proper coroutine cancellation.
148-172: State ordering issue resolved.The previous concern about updating state before clipboard copy has been correctly fixed. The clipboard operation now occurs first (lines 151-154), followed by the state update (lines 156-161). Both success and failure paths appropriately manage the
copiedflag.composeApp/src/commonMain/kotlin/zed/rainxch/githubstore/feature/auth/data/repository/AuthenticationRepositoryImpl.kt (4)
84-89: Token verification flow improves robustness.The new
saveTokenWithVerificationfunction adds a critical verification step after saving the token, ensuring persistence succeeds before proceeding. This prevents silent failures in the token save path.
122-134: Exception types now consistent across error branches.The previous issue with using
CancellationExceptionfor user-actionable errors (access_denied,expired_token) has been correctly resolved. All similar error conditions now throw regularExceptionconsistently.
218-219: Cancellation handling correctly added.The previous issue with the retry loop catching
CancellationExceptionhas been properly resolved. Cancellation exceptions are now rethrown immediately, allowing proper coroutine cancellation.
73-77: Timeout handling correctly improved.The previous issue with using
CancellationExceptionfor timeouts has been properly resolved by switching toTimeoutException. This correctly distinguishes timeout conditions from coroutine cancellation, andjava.util.concurrent.TimeoutExceptionis appropriately available in this JVM+Android project. The implementation correctly uses wall-clock time comparison and allows cooperative cancellation viaisActivechecks.
1 participant
This commit completely refactors the device token polling mechanism for GitHub authentication to improve its robustness, error handling, and resilience.
Key changes include:
remainingMs) is replaced with aSystem.currentTimeMillis()check against a start time, ensuring a more reliable timeout. The loop now runs as long as the coroutine isisActive.isNetworkError()function to classify a wider range of network-related exceptions.slow_down,access_denied,expired_token, andbad_verification_codewith clearer user-facing messages.slow_downresponse now progressively increases the polling interval.saveTokenWithVerificationfunction that attempts to save the token and then immediately verifies its presence in the data source, retrying on failure.Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.