feat(auth): Implement session expiration handling and improved login …#268
Merged
rainxchzed merged 5 commits intomainfrom Feb 28, 2026
Merged
feat(auth): Implement session expiration handling and improved login …#268rainxchzed merged 5 commits intomainfrom
rainxchzed merged 5 commits intomainfrom
Conversation
…flow This commit introduces a mechanism to detect and handle expired GitHub sessions and enhances the device authentication UI with countdown timers and better error categorization. - **feat(auth)**: Added `SessionExpiredDialog` to notify users when their session is invalid or revoked. - **feat(auth)**: Integrated `UnauthorizedInterceptor` (401 handler) into the Ktor `HttpClient` to automatically trigger session expiration events. - **feat(auth)**: Added a countdown timer to the device login screen to show remaining time for code verification. - **feat(auth)**: Introduced `SkipLogin` (Continue as Guest) option to the authentication flow. - **feat(auth)**: Improved error handling in `AuthenticationViewModel` with specific recovery hints (e.g., connection issues, denied access). - **refactor(core)**: Updated `AuthenticationState` and `TokenStore` to manage session expiration events and token lifecycle (including `saved_at` timestamp). - **refactor(cache)**: Added `clearAll()` to `CacheManager` to ensure all local data is purged upon logout or session expiration. - **i18n**: Added string resources for session expiration, auth hints, and logout notes. - **chore**: Updated `LogoutDialog` to include a note about revoking access via GitHub settings.
Contributor
WalkthroughAdds session-expiration detection and UI: tokens are timestamped and expiry-checked, an UnauthorizedInterceptor notifies AuthenticationState on 401, AuthenticationState emits a sessionExpiredEvent, MainViewModel shows a SessionExpiredDialog, device-login countdown and guest flow added, and cache clear on logout. Changes
Sequence DiagramsequenceDiagram
participant User
participant App as App (Main)
participant VM as MainViewModel
participant AuthState as AuthenticationState
participant Client as HTTP Client
participant Server as GitHub Server
User->>Client: API request
Client->>Server: HTTP request
Server-->>Client: 401 Unauthorized
Client->>AuthState: notifySessionExpired()
AuthState->>AuthState: emit sessionExpiredEvent
AuthState->>VM: sessionExpiredEvent collected
VM->>App: set showSessionExpiredDialog = true
App->>User: render SessionExpiredDialog
alt Sign in again
User->>App: onSignIn
App->>VM: DismissSessionExpiredDialog
App->>App: navigate to AuthenticationScreen
else Dismiss
User->>App: onDismiss
App->>VM: DismissSessionExpiredDialog
App->>App: hide dialog
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
feature/auth/presentation/src/commonMain/kotlin/zed/rainxch/auth/presentation/AuthenticationViewModel.kt (1)
222-236:⚠️ Potential issue | 🟠 MajorBug:
copyCoderesetsremainingSecondsto 0.When creating
DevicePrompt(start)withoutremainingSeconds, it defaults to 0, causing the countdown display to disappear after copying the code. The current countdown value should be preserved.🐛 Proposed fix to preserve countdown
_state.update { + val currentRemaining = (it.loginState as? AuthLoginState.DevicePrompt)?.remainingSeconds ?: 0 it.copy( - loginState = AuthLoginState.DevicePrompt(start), + loginState = AuthLoginState.DevicePrompt(start, currentRemaining), copied = true ) } } catch (e: Exception) { logger.debug("⚠️ Failed to copy to clipboard: ${e.message}") _state.update { + val currentRemaining = (it.loginState as? AuthLoginState.DevicePrompt)?.remainingSeconds ?: 0 it.copy( - loginState = AuthLoginState.DevicePrompt(start), + loginState = AuthLoginState.DevicePrompt(start, currentRemaining), copied = false ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/auth/presentation/src/commonMain/kotlin/zed/rainxch/auth/presentation/AuthenticationViewModel.kt` around lines 222 - 236, The bug is that AuthenticationViewModel's copyCode path recreates AuthLoginState.DevicePrompt with only start (so remainingSeconds defaults to 0), causing the countdown to vanish; fix it by reading the current remainingSeconds from _state.value.loginState (if it is a DevicePrompt) or from the existing state and pass that remainingSeconds into AuthLoginState.DevicePrompt(start, remainingSeconds) when updating _state; modify the two places in the try and catch where you call AuthLoginState.DevicePrompt(start) to instead preserve and supply the existing remainingSeconds.
🧹 Nitpick comments (4)
feature/auth/presentation/src/commonMain/kotlin/zed/rainxch/auth/presentation/AuthenticationRoot.kt (1)
252-269: Consider zero-padding minutes for visual consistency.The format string
"%d:%02d"produces asymmetric output like5:30instead of05:30. While functional, zero-padding minutes ("%02d:%02d") would provide consistent width and improve readability.♻️ Optional: Zero-pad minutes
val formatted = remember(minutes, seconds) { - "%d:%02d".format(minutes, seconds) + "%02d:%02d".format(minutes, seconds) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/auth/presentation/src/commonMain/kotlin/zed/rainxch/auth/presentation/AuthenticationRoot.kt` around lines 252 - 269, The countdown formatting in the AuthenticationRoot composable uses "%d:%02d" to produce values like "5:30"; update the formatting used when computing formatted (the remember block that formats minutes and seconds from authState.remainingSeconds) to use "%02d:%02d" so minutes are zero-padded for consistent width (keep the existing remember(minutes, seconds) usage and the same stringResource Res.string.auth_code_expires_in in the Text).feature/auth/presentation/src/commonMain/kotlin/zed/rainxch/auth/presentation/AuthenticationViewModel.kt (1)
182-196: String-based error categorization is fragile but acceptable.The approach works for common cases, but relying on substrings in error messages is brittle—messages may vary across platforms, locales, or library versions. Consider defining typed error classes in the repository layer if more robust error handling is needed in the future.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/auth/presentation/src/commonMain/kotlin/zed/rainxch/auth/presentation/AuthenticationViewModel.kt` around lines 182 - 196, The categorizeError function uses fragile substring matching on throwable.message (in private suspend fun categorizeError) which can break across platforms/locales; replace or augment this by adding typed error classes or sealed error types in the repository layer (e.g., AuthError subclasses) and have repository methods throw or return those typed errors so AuthenticationViewModel.categorizeError can pattern-match on the error type instead of message content; update callers to propagate the new error types and map each AuthError (timeout, network, expired, denied) to the existing user-facing message/getString logic.composeApp/src/commonMain/kotlin/zed/rainxch/githubstore/app/components/SessionExpiredDialog.kt (2)
45-55: Consider usingonSurfaceVariantfor secondary text.
MaterialTheme.colorScheme.outlineis semantically intended for borders and outlines rather than text content. For secondary/supporting text,onSurfaceVariantis more appropriate and provides better accessibility contrast guarantees.♻️ Suggested change
Text( text = stringResource(Res.string.session_expired_message), style = MaterialTheme.typography.bodyMedium, - color = MaterialTheme.colorScheme.outline + color = MaterialTheme.colorScheme.onSurfaceVariant )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composeApp/src/commonMain/kotlin/zed/rainxch/githubstore/app/components/SessionExpiredDialog.kt` around lines 45 - 55, The secondary text in SessionExpiredDialog is using MaterialTheme.colorScheme.outline (meant for borders) instead of the semantic color for supporting text; update the Text whose text is stringResource(Res.string.session_expired_hint) to use MaterialTheme.colorScheme.onSurfaceVariant instead of MaterialTheme.colorScheme.outline (leave other text colors unchanged) so the component uses the correct semantic color for secondary text and improves accessibility contrast.
58-75: Consider removing explicit text colors in buttons.Material3
ButtonandTextButtonautomatically provide correct content colors viaLocalContentColor. ForButton, text is alreadyonPrimary; forTextButton, text defaults toprimary. These explicit color overrides are either redundant or deviate from Material3 defaults.If the
onSurfacecolor for "Continue as Guest" is intentional to de-emphasize it, this is fine—just noting it differs from Material3 default styling.♻️ Suggested simplification
confirmButton = { Button(onClick = onSignIn) { Text( text = stringResource(Res.string.sign_in_again), - style = MaterialTheme.typography.bodySmall, - color = MaterialTheme.colorScheme.onPrimary + style = MaterialTheme.typography.bodySmall ) } }, dismissButton = { TextButton(onClick = onDismiss) { Text( text = stringResource(Res.string.continue_as_guest), - style = MaterialTheme.typography.bodySmall, - color = MaterialTheme.colorScheme.onSurface + style = MaterialTheme.typography.bodySmall ) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composeApp/src/commonMain/kotlin/zed/rainxch/githubstore/app/components/SessionExpiredDialog.kt` around lines 58 - 75, In SessionExpiredDialog update the confirmButton and dismissButton content so they stop overriding the default Material3 content colors: remove the explicit color = MaterialTheme.colorScheme.onPrimary from the Button's Text in confirmButton and remove color = MaterialTheme.colorScheme.onSurface from the TextButton's Text in dismissButton unless the de-emphasized onSurface styling for "Continue as guest" is intentional; this lets Button/TextButton use LocalContentColor and MaterialTheme defaults (locate the confirmButton/dismissButton lambdas with Button and TextButton in SessionExpiredDialog to apply the change).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@core/data/src/commonMain/kotlin/zed/rainxch/core/data/data_source/impl/DefaultTokenStore.kt`:
- Around line 60-66: The isTokenExpired method in DefaultTokenStore uses
JVM-only System.currentTimeMillis(); replace that call with the multiplatform
clock API (e.g., use kotlin.time.Clock.System.now().toEpochMilliseconds() or the
project’s CacheManager helper if it exposes a now/epoch-millis method) so the
check is multiplatform-safe—update DefaultTokenStore.isTokenExpired to compute
expiresAtMillis using Clock.System.now().toEpochMilliseconds() (or
CacheManager’s equivalent) and remove the System.currentTimeMillis() dependency.
In
`@core/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/AuthenticationStateImpl.kt`:
- Around line 30-33: AuthenticationStateImpl.notifySessionExpired is currently
unsafe for concurrent 401 handling: add a Mutex (e.g., a private val
sessionMutex = Mutex()) and wrap the body of notifySessionExpired in
sessionMutex.withLock { ... } so the tokenStore.clear() and
_sessionExpiredEvent.emit(Unit) are serialized; inside the lock first check
tokenStore.token (or equivalent accessor) for null/empty and return early if
already cleared to make the method idempotent, then clear the token and emit the
event once. Ensure imports and coroutine context usage are correct for withLock
and suspend usage.
---
Outside diff comments:
In
`@feature/auth/presentation/src/commonMain/kotlin/zed/rainxch/auth/presentation/AuthenticationViewModel.kt`:
- Around line 222-236: The bug is that AuthenticationViewModel's copyCode path
recreates AuthLoginState.DevicePrompt with only start (so remainingSeconds
defaults to 0), causing the countdown to vanish; fix it by reading the current
remainingSeconds from _state.value.loginState (if it is a DevicePrompt) or from
the existing state and pass that remainingSeconds into
AuthLoginState.DevicePrompt(start, remainingSeconds) when updating _state;
modify the two places in the try and catch where you call
AuthLoginState.DevicePrompt(start) to instead preserve and supply the existing
remainingSeconds.
---
Nitpick comments:
In
`@composeApp/src/commonMain/kotlin/zed/rainxch/githubstore/app/components/SessionExpiredDialog.kt`:
- Around line 45-55: The secondary text in SessionExpiredDialog is using
MaterialTheme.colorScheme.outline (meant for borders) instead of the semantic
color for supporting text; update the Text whose text is
stringResource(Res.string.session_expired_hint) to use
MaterialTheme.colorScheme.onSurfaceVariant instead of
MaterialTheme.colorScheme.outline (leave other text colors unchanged) so the
component uses the correct semantic color for secondary text and improves
accessibility contrast.
- Around line 58-75: In SessionExpiredDialog update the confirmButton and
dismissButton content so they stop overriding the default Material3 content
colors: remove the explicit color = MaterialTheme.colorScheme.onPrimary from the
Button's Text in confirmButton and remove color =
MaterialTheme.colorScheme.onSurface from the TextButton's Text in dismissButton
unless the de-emphasized onSurface styling for "Continue as guest" is
intentional; this lets Button/TextButton use LocalContentColor and MaterialTheme
defaults (locate the confirmButton/dismissButton lambdas with Button and
TextButton in SessionExpiredDialog to apply the change).
In
`@feature/auth/presentation/src/commonMain/kotlin/zed/rainxch/auth/presentation/AuthenticationRoot.kt`:
- Around line 252-269: The countdown formatting in the AuthenticationRoot
composable uses "%d:%02d" to produce values like "5:30"; update the formatting
used when computing formatted (the remember block that formats minutes and
seconds from authState.remainingSeconds) to use "%02d:%02d" so minutes are
zero-padded for consistent width (keep the existing remember(minutes, seconds)
usage and the same stringResource Res.string.auth_code_expires_in in the Text).
In
`@feature/auth/presentation/src/commonMain/kotlin/zed/rainxch/auth/presentation/AuthenticationViewModel.kt`:
- Around line 182-196: The categorizeError function uses fragile substring
matching on throwable.message (in private suspend fun categorizeError) which can
break across platforms/locales; replace or augment this by adding typed error
classes or sealed error types in the repository layer (e.g., AuthError
subclasses) and have repository methods throw or return those typed errors so
AuthenticationViewModel.categorizeError can pattern-match on the error type
instead of message content; update callers to propagate the new error types and
map each AuthError (timeout, network, expired, denied) to the existing
user-facing message/getString logic.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
composeApp/src/commonMain/kotlin/zed/rainxch/githubstore/Main.ktcomposeApp/src/commonMain/kotlin/zed/rainxch/githubstore/MainAction.ktcomposeApp/src/commonMain/kotlin/zed/rainxch/githubstore/MainState.ktcomposeApp/src/commonMain/kotlin/zed/rainxch/githubstore/MainViewModel.ktcomposeApp/src/commonMain/kotlin/zed/rainxch/githubstore/app/components/SessionExpiredDialog.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/cache/CacheManager.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/data_source/TokenStore.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/data_source/impl/DefaultTokenStore.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/di/SharedModule.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/dto/GithubDeviceTokenSuccessDto.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/network/GitHubClientProvider.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/network/HttpClientFactory.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/network/interceptor/UnauthorizedInterceptor.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/AuthenticationStateImpl.ktcore/domain/src/commonMain/kotlin/zed/rainxch/core/domain/repository/AuthenticationState.ktcore/presentation/src/commonMain/composeResources/values/strings.xmlfeature/auth/presentation/src/commonMain/kotlin/zed/rainxch/auth/presentation/AuthenticationAction.ktfeature/auth/presentation/src/commonMain/kotlin/zed/rainxch/auth/presentation/AuthenticationRoot.ktfeature/auth/presentation/src/commonMain/kotlin/zed/rainxch/auth/presentation/AuthenticationViewModel.ktfeature/auth/presentation/src/commonMain/kotlin/zed/rainxch/auth/presentation/model/AuthLoginState.ktfeature/profile/data/src/commonMain/kotlin/zed/rainxch/profile/data/repository/ProfileRepositoryImpl.ktfeature/profile/presentation/src/commonMain/kotlin/zed/rainxch/profile/presentation/components/LogoutDialog.kt
core/data/src/commonMain/kotlin/zed/rainxch/core/data/data_source/impl/DefaultTokenStore.kt
Show resolved
Hide resolved
core/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/AuthenticationStateImpl.kt
Show resolved
Hide resolved
This commit enhances the authentication lifecycle and background update synchronization. It introduces thread-safe session expiration handling, improves the reliability of background update checks on Android, and refines the authentication UI and token management. - **feat(android)**: Updated `UpdateScheduler` to include an immediate one-time update check in addition to periodic work, ensuring users see updates sooner after initialization. - **fix(core)**: Implemented a `Mutex` in `AuthenticationStateImpl` to prevent race conditions during session expiration and token clearing. - **refactor(core)**: Replaced `System.currentTimeMillis()` with `Clock.System` from kotlinx-datetime in `DefaultTokenStore` for better multiplatform compatibility. - **fix(auth)**: Fixed a bug in `AuthenticationViewModel` where `remainingSeconds` was reset to 0 during clipboard copy actions in the device prompt state. - **ui(auth)**: Improved the expiration timer format in `AuthenticationRoot` to use padded zeros (e.g., "05:01" instead of "5:01"). - **chore**: Updated `UpdateScheduler` to always use `ExistingPeriodicWorkPolicy.UPDATE` to ensure configuration changes are applied.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/UpdateScheduler.kt (2)
65-70: Minor: Log message doesn't reflect all cancelled work.The log message still says "Cancelled periodic update checks" but now also cancels the immediate check. Consider updating for clarity.
📝 Suggested log update
fun cancel(context: Context) { WorkManager.getInstance(context) .cancelUniqueWork(UpdateCheckWorker.WORK_NAME) WorkManager.getInstance(context) .cancelUniqueWork(IMMEDIATE_CHECK_WORK_NAME) - Logger.i { "UpdateScheduler: Cancelled periodic update checks" } + Logger.i { "UpdateScheduler: Cancelled all update checks" } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/UpdateScheduler.kt` around lines 65 - 70, Update the log in cancel() to accurately reflect both cancelled jobs: when WorkManager.cancelUniqueWork is called for UpdateCheckWorker.WORK_NAME and IMMEDIATE_CHECK_WORK_NAME, change the Logger.i message to something like "Cancelled periodic and immediate update checks" (or similarly explicit) so it reflects both cancelled works from the cancel() method.
40-60: Consider caching WorkManager instance.
WorkManager.getInstance(context)is called multiple times. A local variable would reduce repetition.♻️ Optional cleanup
+ val workManager = WorkManager.getInstance(context) + // UPDATE replaces any existing schedule so new intervals and code changes take effect - WorkManager.getInstance(context) + workManager .enqueueUniquePeriodicWork( uniqueWorkName = UpdateCheckWorker.WORK_NAME, existingPeriodicWorkPolicy = ExistingPeriodicWorkPolicy.UPDATE, request = request ) // Run an immediate one-time check so users get notified sooner // rather than waiting up to intervalHours for the first periodic run. // Uses KEEP policy so it doesn't re-enqueue if one is already pending. val immediateRequest = OneTimeWorkRequestBuilder<UpdateCheckWorker>() .setConstraints(constraints) .setInitialDelay(1, TimeUnit.MINUTES) .build() - WorkManager.getInstance(context) + workManager .enqueueUniqueWork( IMMEDIATE_CHECK_WORK_NAME, ExistingWorkPolicy.KEEP, immediateRequest )Same pattern can be applied in
cancel().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/UpdateScheduler.kt` around lines 40 - 60, Cache the WorkManager instance instead of calling WorkManager.getInstance(context) repeatedly: create a local val (e.g., workManager = WorkManager.getInstance(context)) and use that variable for enqueueUniquePeriodicWork (the call that uses UpdateCheckWorker.WORK_NAME and request), enqueueUniqueWork (the call that uses IMMEDIATE_CHECK_WORK_NAME and immediateRequest), and in the cancel() method to avoid duplication and make the code clearer.feature/auth/presentation/src/commonMain/kotlin/zed/rainxch/auth/presentation/AuthenticationRoot.kt (1)
108-114: Consider adding a guest option in DevicePrompt state for consistency.The
ErrorandLoggedOutstates both offer a "Continue as guest" option, butDevicePromptdoes not. If a user initiates login but changes their mind mid-flow, they currently have no easy way to abandon authentication and proceed as a guest.This may be intentional, but adding a secondary "Cancel" or "Continue as guest" action in
StateDevicePromptwould provide a consistent escape hatch across all states.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/auth/presentation/src/commonMain/kotlin/zed/rainxch/auth/presentation/AuthenticationRoot.kt` around lines 108 - 114, AuthLoginState.DevicePrompt currently renders StateDevicePrompt without a guest/cancel fallback; add a secondary "Continue as guest" or "Cancel" action to StateDevicePrompt so users can abandon the device-prompt flow. Update the callsite in AuthenticationRoot where AuthLoginState.DevicePrompt is handled (the StateDevicePrompt invocation) to pass a new lambda via onAction or a dedicated onCancel/onGuest parameter, and implement that action inside StateDevicePrompt to dispatch the same guest action used by Error/LoggedOut (reuse the existing guest action identifier or call used by those states) so behavior is consistent across Error, LoggedOut and DevicePrompt flows.feature/auth/presentation/src/commonMain/kotlin/zed/rainxch/auth/presentation/AuthenticationViewModel.kt (1)
86-88: Cancel in-flight auth work before guest navigation.On Line 86,
SkipLoginemits navigation immediately, but the auth flow/timer may still be active until disposal. Explicitly cancel active jobs here for deterministic cleanup.Suggested patch
@@ - private var countdownJob: Job? = null + private var countdownJob: Job? = null + private var loginJob: Job? = null @@ - private fun startLogin() { - viewModelScope.launch { + private fun startLogin() { + loginJob?.cancel() + loginJob = viewModelScope.launch { @@ AuthenticationAction.SkipLogin -> { + countdownJob?.cancel() + loginJob?.cancel() + _state.update { it.copy(copied = false) } _events.trySend(AuthenticationEvents.OnNavigateToMain) } @@ override fun onCleared() { super.onCleared() countdownJob?.cancel() + loginJob?.cancel() }Based on learnings: "AuthenticationViewModel should manage the complete device flow lifecycle including starting the flow, polling for completion, and handling user cancellation".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/auth/presentation/src/commonMain/kotlin/zed/rainxch/auth/presentation/AuthenticationViewModel.kt` around lines 86 - 88, In the AuthenticationAction.SkipLogin branch, cancel any active device/auth flow job(s) before emitting navigation: locate the handler for AuthenticationAction.SkipLogin (the branch containing _events.trySend(AuthenticationEvents.OnNavigateToMain)) and invoke the ViewModel's cancellation routine (e.g., call cancelAuthFlow(), authFlowJob?.cancel(), or cancel the relevant CoroutineScope/job/managers used by startDeviceFlow/polling) and clear related state, then call _events.trySend(AuthenticationEvents.OnNavigateToMain) so the in-flight auth work is deterministically cleaned up before navigating.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@feature/auth/presentation/src/commonMain/kotlin/zed/rainxch/auth/presentation/AuthenticationViewModel.kt`:
- Around line 223-237: The update currently overwrites loginState
unconditionally to AuthLoginState.DevicePrompt (in the _state.update block),
which can rollback other states; change the update to preserve the existing
it.loginState except when it is already a DevicePrompt — only then construct a
new AuthLoginState.DevicePrompt(start, currentRemaining); otherwise leave
loginState untouched and only set copied = true/false. Locate _state.update,
AuthLoginState.DevicePrompt, copied and start to implement this
conditional-preserve logic.
---
Nitpick comments:
In
`@core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/UpdateScheduler.kt`:
- Around line 65-70: Update the log in cancel() to accurately reflect both
cancelled jobs: when WorkManager.cancelUniqueWork is called for
UpdateCheckWorker.WORK_NAME and IMMEDIATE_CHECK_WORK_NAME, change the Logger.i
message to something like "Cancelled periodic and immediate update checks" (or
similarly explicit) so it reflects both cancelled works from the cancel()
method.
- Around line 40-60: Cache the WorkManager instance instead of calling
WorkManager.getInstance(context) repeatedly: create a local val (e.g.,
workManager = WorkManager.getInstance(context)) and use that variable for
enqueueUniquePeriodicWork (the call that uses UpdateCheckWorker.WORK_NAME and
request), enqueueUniqueWork (the call that uses IMMEDIATE_CHECK_WORK_NAME and
immediateRequest), and in the cancel() method to avoid duplication and make the
code clearer.
In
`@feature/auth/presentation/src/commonMain/kotlin/zed/rainxch/auth/presentation/AuthenticationRoot.kt`:
- Around line 108-114: AuthLoginState.DevicePrompt currently renders
StateDevicePrompt without a guest/cancel fallback; add a secondary "Continue as
guest" or "Cancel" action to StateDevicePrompt so users can abandon the
device-prompt flow. Update the callsite in AuthenticationRoot where
AuthLoginState.DevicePrompt is handled (the StateDevicePrompt invocation) to
pass a new lambda via onAction or a dedicated onCancel/onGuest parameter, and
implement that action inside StateDevicePrompt to dispatch the same guest action
used by Error/LoggedOut (reuse the existing guest action identifier or call used
by those states) so behavior is consistent across Error, LoggedOut and
DevicePrompt flows.
In
`@feature/auth/presentation/src/commonMain/kotlin/zed/rainxch/auth/presentation/AuthenticationViewModel.kt`:
- Around line 86-88: In the AuthenticationAction.SkipLogin branch, cancel any
active device/auth flow job(s) before emitting navigation: locate the handler
for AuthenticationAction.SkipLogin (the branch containing
_events.trySend(AuthenticationEvents.OnNavigateToMain)) and invoke the
ViewModel's cancellation routine (e.g., call cancelAuthFlow(),
authFlowJob?.cancel(), or cancel the relevant CoroutineScope/job/managers used
by startDeviceFlow/polling) and clear related state, then call
_events.trySend(AuthenticationEvents.OnNavigateToMain) so the in-flight auth
work is deterministically cleaned up before navigating.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/UpdateScheduler.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/data_source/impl/DefaultTokenStore.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/AuthenticationStateImpl.ktfeature/auth/presentation/src/commonMain/kotlin/zed/rainxch/auth/presentation/AuthenticationRoot.ktfeature/auth/presentation/src/commonMain/kotlin/zed/rainxch/auth/presentation/AuthenticationViewModel.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- core/data/src/commonMain/kotlin/zed/rainxch/core/data/data_source/impl/DefaultTokenStore.kt
Comment on lines
+223
to
237
| val currentRemaining = (it.loginState as? AuthLoginState.DevicePrompt)?.remainingSeconds ?: 0 | ||
|
|
||
| it.copy( | ||
| loginState = AuthLoginState.DevicePrompt(start), | ||
| loginState = AuthLoginState.DevicePrompt(start, currentRemaining), | ||
| copied = true | ||
| ) | ||
| } | ||
| } catch (e: Exception) { | ||
| logger.debug("⚠️ Failed to copy to clipboard: ${e.message}") | ||
| _state.update { | ||
| val currentRemaining = (it.loginState as? AuthLoginState.DevicePrompt)?.remainingSeconds ?: 0 | ||
|
|
||
| it.copy( | ||
| loginState = AuthLoginState.DevicePrompt(start), | ||
| loginState = AuthLoginState.DevicePrompt(start, currentRemaining), | ||
| copied = false |
Contributor
There was a problem hiding this comment.
Avoid forcing DevicePrompt during copy-result updates.
On Line 226 and Line 236, state is always rewritten to AuthLoginState.DevicePrompt. If state changes between click and handler execution, this can rollback Error/LoggedIn unexpectedly.
Suggested patch
@@
_state.update {
- val currentRemaining = (it.loginState as? AuthLoginState.DevicePrompt)?.remainingSeconds ?: 0
-
- it.copy(
- loginState = AuthLoginState.DevicePrompt(start, currentRemaining),
- copied = true
- )
+ val prompt = it.loginState as? AuthLoginState.DevicePrompt ?: return@update it
+ it.copy(loginState = prompt, copied = true)
}
@@
_state.update {
- val currentRemaining = (it.loginState as? AuthLoginState.DevicePrompt)?.remainingSeconds ?: 0
-
- it.copy(
- loginState = AuthLoginState.DevicePrompt(start, currentRemaining),
- copied = false
- )
+ val prompt = it.loginState as? AuthLoginState.DevicePrompt ?: return@update it
+ it.copy(loginState = prompt, copied = false)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@feature/auth/presentation/src/commonMain/kotlin/zed/rainxch/auth/presentation/AuthenticationViewModel.kt`
around lines 223 - 237, The update currently overwrites loginState
unconditionally to AuthLoginState.DevicePrompt (in the _state.update block),
which can rollback other states; change the update to preserve the existing
it.loginState except when it is already a DevicePrompt — only then construct a
new AuthLoginState.DevicePrompt(start, currentRemaining); otherwise leave
loginState untouched and only set copied = true/false. Locate _state.update,
AuthLoginState.DevicePrompt, copied and start to implement this
conditional-preserve logic.
This commit introduces a "hot" indicator (🔥) for repositories updated within the last week and cleans up unused resources and time formatting logic. - **feat(ui)**: Updated `RepositoryCard` to prepend a fire emoji to the release date if the repository was updated within the last 7 days. - **refactor(core)**: Introduced `hasWeekNotPassed` utility in `TimeFormatters.kt` to check repository update freshness. - **refactor(core)**: Removed unused `formatUpdatedAt` functions and simplified `TimeFormatters.kt`. - **chore(android)**: Removed default Android launcher background and foreground vector drawables. - **chore**: Cleaned up unused imports and Material3 Experimental APIs in `RepositoryCard.kt`.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
core/presentation/src/commonMain/kotlin/zed/rainxch/core/presentation/components/RepositoryCard.kt (1)
240-249: Consider consolidating timestamp parsing.Both
hasWeekNotPassed()andformatReleasedAt()independently parse the sameupdatedAtISO string. While functionally correct, this performs duplicate parsing work.Also, if
updatedAtis ever malformed, this will throw during composition and crash the card.Proposed refactor to parse once
A cleaner approach would be to have a single utility that returns both the "is hot" flag and the formatted string, or to pass the parsed
Instantto both functions. This also centralizes error handling.val releasedAtText = remember(discoveryRepository.repository.updatedAt) { val isHot = hasWeekNotPassed(discoveryRepository.repository.updatedAt) val prefix = if (isHot) "🔥 " else "" prefix // formatReleasedAt would be called separately as it's `@Composable` }Alternatively, consider adding try-catch in
hasWeekNotPassed(see TimeFormatters.kt review) to prevent crashes from malformed data.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/presentation/src/commonMain/kotlin/zed/rainxch/core/presentation/components/RepositoryCard.kt` around lines 240 - 249, RepositoryCard currently calls hasWeekNotPassed(...) and formatReleasedAt(...) with the same discoveryRepository.repository.updatedAt ISO string, causing duplicate parsing and crash risk on malformed timestamps; fix by parsing updatedAt once (e.g., within RepositoryCard using remember) into an Instant (or a small data object) and pass that parsed value into hasWeekNotPassed and formatReleasedAt (or create a single utility that returns both isHot and formatted string), and ensure parsing is wrapped with try/catch/fallback so malformed updatedAt values do not throw during composition (see TimeFormatters.kt for parsing helpers).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@core/presentation/src/commonMain/kotlin/zed/rainxch/core/presentation/utils/TimeFormatters.kt`:
- Line 16: The code calls Instant.parse(isoInstant) directly (assigning to
updated), which can throw on malformed input; wrap the parse in a try/catch
(catch DateTimeParseException/IllegalArgumentException) or use runCatching to
safely parse and handle failures, and propagate a nullable Instant or a sensible
fallback instead of letting the exception escape. Update the TimeFormatters.kt
logic that sets the updated variable (Instant.parse) to return null or a default
formatted string on parse failure, and adjust any callers to handle the
nullable/alternate result.
- Around line 15-21: The hasWeekNotPassed function uses experimental time APIs
(Instant, Clock.System, days) but lacks the `@OptIn`(ExperimentalTime::class)
annotation; add the `@OptIn`(ExperimentalTime::class) annotation either directly
above the hasWeekNotPassed function or at the file level so the compiler
recognizes experimental usage, ensuring consistency with formatReleasedAt and
formatAddedAt which already opt into ExperimentalTime.
---
Nitpick comments:
In
`@core/presentation/src/commonMain/kotlin/zed/rainxch/core/presentation/components/RepositoryCard.kt`:
- Around line 240-249: RepositoryCard currently calls hasWeekNotPassed(...) and
formatReleasedAt(...) with the same discoveryRepository.repository.updatedAt ISO
string, causing duplicate parsing and crash risk on malformed timestamps; fix by
parsing updatedAt once (e.g., within RepositoryCard using remember) into an
Instant (or a small data object) and pass that parsed value into
hasWeekNotPassed and formatReleasedAt (or create a single utility that returns
both isHot and formatted string), and ensure parsing is wrapped with
try/catch/fallback so malformed updatedAt values do not throw during composition
(see TimeFormatters.kt for parsing helpers).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
composeApp/src/androidMain/res/drawable-v24/ic_launcher_foreground.xmlcomposeApp/src/androidMain/res/drawable/ic_launcher_background.xmlcore/presentation/src/commonMain/kotlin/zed/rainxch/core/presentation/components/RepositoryCard.ktcore/presentation/src/commonMain/kotlin/zed/rainxch/core/presentation/utils/TimeFormatters.kt
💤 Files with no reviewable changes (2)
- composeApp/src/androidMain/res/drawable/ic_launcher_background.xml
- composeApp/src/androidMain/res/drawable-v24/ic_launcher_foreground.xml
core/presentation/src/commonMain/kotlin/zed/rainxch/core/presentation/utils/TimeFormatters.kt
Show resolved
Hide resolved
core/presentation/src/commonMain/kotlin/zed/rainxch/core/presentation/utils/TimeFormatters.kt
Outdated
Show resolved
Hide resolved
… languages This commit adds localized string resources for session expiration dialogs and authentication errors across several languages (Turkish, Spanish, Chinese, French, Hindi, Italian, Japanese, Korean, Polish, Bengali, and Russian). It also includes a small fix to prevent crashes during date parsing. - **i18n**: Added strings for session expiration titles, messages, and hints (`session_expired_title`, `session_expired_message`, etc.). - **i18n**: Added strings for authentication lifecycle and errors, including code expiration and connection hints (`auth_code_expires_in`, `auth_error_code_expired`, `auth_hint_denied`). - **fix(core)**: Wrapped `Instant.parse` in a try-catch block within `TimeFormatters.kt` to gracefully handle invalid ISO strings by returning `false` instead of throwing an `IllegalArgumentException`.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
core/presentation/src/commonMain/composeResources/values-it/strings-it.xml (1)
417-417: Use a more stable revocation reference text.Line 417 hard-codes
GitHub Settings > Applications; this can become outdated or mismatch localized GitHub UI labels. Consider a generic instruction or direct revocation URL wording instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/presentation/src/commonMain/composeResources/values-it/strings-it.xml` at line 417, The localized string logout_revocation_note currently hard-codes "GitHub Settings > Applications", which can become inaccurate; update the value of logout_revocation_note in strings-it.xml to use a stable, generic instruction or a revocation URL phrasing (e.g., "Per revocare completamente l'accesso, visita le impostazioni del tuo account GitHub e rimuovi l'applicazione" or "visita la pagina di gestione delle applicazioni del tuo account GitHub") so the text does not rely on specific UI labels and remains correct across locales and UI changes.core/presentation/src/commonMain/composeResources/values-ru/strings-ru.xml (1)
381-381: Consider localizing the GitHub navigation path for Russian users.Line 381 keeps
GitHub Settings > Applicationsin English. A RU phrasing (or language-neutral wording) would be clearer in this locale.💡 Suggested wording tweak
- <string name="logout_revocation_note">Это очистит вашу локальную сессию и кэшированные данные. Чтобы полностью отозвать доступ, перейдите в GitHub Settings > Applications.</string> + <string name="logout_revocation_note">Это очистит вашу локальную сессию и кэшированные данные. Чтобы полностью отозвать доступ, перейдите в настройки GitHub в раздел Applications.</string>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/presentation/src/commonMain/composeResources/values-ru/strings-ru.xml` at line 381, The string resource logout_revocation_note contains an English navigation path "GitHub Settings > Applications" in the Russian locale; update the value of logout_revocation_note in values-ru/strings-ru.xml to use a Russian phrasing or a language-neutral instruction (for example, "Настройки GitHub → Приложения" or "в настройках вашего аккаунта GitHub, раздел «Приложения»") while keeping the explanatory text intact so Russian users see a localized navigation hint.core/presentation/src/commonMain/kotlin/zed/rainxch/core/presentation/utils/TimeFormatters.kt (1)
30-31: Consider adding similar error handling toformatReleasedAtfor consistency.While outside this PR's changed scope,
Instant.parse(isoInstant)on line 31 could throwIllegalArgumentExceptionon malformed input, similar to the issue just fixed inhasWeekNotPassed. For consistency and robustness, consider applying the same defensive pattern here.Proposed fix for consistency
`@OptIn`(ExperimentalTime::class) `@Composable` fun formatReleasedAt(isoInstant: String): String { - val updated = Instant.parse(isoInstant) + val updated = try { + Instant.parse(isoInstant) + } catch (_: IllegalArgumentException) { + return stringResource(Res.string.released_just_now) // or a fallback like "Unknown" + } val now = Instant.fromEpochMilliseconds(Clock.System.now().toEpochMilliseconds())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/presentation/src/commonMain/kotlin/zed/rainxch/core/presentation/utils/TimeFormatters.kt` around lines 30 - 31, The function formatReleasedAt currently calls Instant.parse(isoInstant) which can throw IllegalArgumentException for malformed input; update formatReleasedAt to catch parsing errors (e.g., wrap Instant.parse in try/catch), handle IllegalArgumentException by returning a sensible fallback (such as the original string, an empty string, or a default formatted value) and/or logging the parse failure, mirroring the defensive pattern used in hasWeekNotPassed; ensure you reference formatReleasedAt and the Instant.parse(isoInstant) call when making the change so behavior remains consistent across the utility.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/presentation/src/commonMain/composeResources/values-pl/strings-pl.xml`:
- Line 379: The string resource logout_revocation_note contains English menu
labels ("GitHub Settings > Applications") inside the Polish file; update the
value of string name="logout_revocation_note" to use Polish localized menu text
(e.g., "Ustawienia GitHub > Aplikacje" or a Polish appropriate localization such
as "Ustawienia GitHub → Aplikacje") so the entire sentence is in Polish and
consistent with values-pl resources.
---
Nitpick comments:
In `@core/presentation/src/commonMain/composeResources/values-it/strings-it.xml`:
- Line 417: The localized string logout_revocation_note currently hard-codes
"GitHub Settings > Applications", which can become inaccurate; update the value
of logout_revocation_note in strings-it.xml to use a stable, generic instruction
or a revocation URL phrasing (e.g., "Per revocare completamente l'accesso,
visita le impostazioni del tuo account GitHub e rimuovi l'applicazione" or
"visita la pagina di gestione delle applicazioni del tuo account GitHub") so the
text does not rely on specific UI labels and remains correct across locales and
UI changes.
In `@core/presentation/src/commonMain/composeResources/values-ru/strings-ru.xml`:
- Line 381: The string resource logout_revocation_note contains an English
navigation path "GitHub Settings > Applications" in the Russian locale; update
the value of logout_revocation_note in values-ru/strings-ru.xml to use a Russian
phrasing or a language-neutral instruction (for example, "Настройки GitHub →
Приложения" or "в настройках вашего аккаунта GitHub, раздел «Приложения»") while
keeping the explanatory text intact so Russian users see a localized navigation
hint.
In
`@core/presentation/src/commonMain/kotlin/zed/rainxch/core/presentation/utils/TimeFormatters.kt`:
- Around line 30-31: The function formatReleasedAt currently calls
Instant.parse(isoInstant) which can throw IllegalArgumentException for malformed
input; update formatReleasedAt to catch parsing errors (e.g., wrap Instant.parse
in try/catch), handle IllegalArgumentException by returning a sensible fallback
(such as the original string, an empty string, or a default formatted value)
and/or logging the parse failure, mirroring the defensive pattern used in
hasWeekNotPassed; ensure you reference formatReleasedAt and the
Instant.parse(isoInstant) call when making the change so behavior remains
consistent across the utility.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
core/presentation/src/commonMain/composeResources/values-bn/strings-bn.xmlcore/presentation/src/commonMain/composeResources/values-es/strings-es.xmlcore/presentation/src/commonMain/composeResources/values-fr/strings-fr.xmlcore/presentation/src/commonMain/composeResources/values-hi/strings-hi.xmlcore/presentation/src/commonMain/composeResources/values-it/strings-it.xmlcore/presentation/src/commonMain/composeResources/values-ja/strings-ja.xmlcore/presentation/src/commonMain/composeResources/values-kr/strings-kr.xmlcore/presentation/src/commonMain/composeResources/values-pl/strings-pl.xmlcore/presentation/src/commonMain/composeResources/values-ru/strings-ru.xmlcore/presentation/src/commonMain/composeResources/values-tr/strings-tr.xmlcore/presentation/src/commonMain/composeResources/values-zh-rCN/strings-zh-rCN.xmlcore/presentation/src/commonMain/kotlin/zed/rainxch/core/presentation/utils/TimeFormatters.kt
| <string name="session_expired_hint">Możesz nadal przeglądać jako gość z ograniczoną liczbą zapytań API.</string> | ||
| <string name="sign_in_again">Zaloguj się ponownie</string> | ||
| <string name="continue_as_guest">Kontynuuj jako gość</string> | ||
| <string name="logout_revocation_note">Spowoduje to wyczyszczenie lokalnej sesji i danych z pamięci podręcznej. Aby całkowicie cofnąć dostęp, odwiedź GitHub Settings > Applications.</string> |
Contributor
There was a problem hiding this comment.
Localize mixed-language phrase in Polish resource.
Line 379 mixes Polish with English menu labels (GitHub Settings > Applications), which is inconsistent in values-pl and can confuse users.
💬 Suggested wording update
- <string name="logout_revocation_note">Spowoduje to wyczyszczenie lokalnej sesji i danych z pamięci podręcznej. Aby całkowicie cofnąć dostęp, odwiedź GitHub Settings > Applications.</string>
+ <string name="logout_revocation_note">Spowoduje to wyczyszczenie lokalnej sesji i danych z pamięci podręcznej. Aby całkowicie cofnąć dostęp, przejdź do GitHub → Ustawienia → Aplikacje.</string>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <string name="logout_revocation_note">Spowoduje to wyczyszczenie lokalnej sesji i danych z pamięci podręcznej. Aby całkowicie cofnąć dostęp, odwiedź GitHub Settings > Applications.</string> | |
| <string name="logout_revocation_note">Spowoduje to wyczyszczenie lokalnej sesji i danych z pamięci podręcznej. Aby całkowicie cofnąć dostęp, przejdź do GitHub → Ustawienia → Aplikacje.</string> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/presentation/src/commonMain/composeResources/values-pl/strings-pl.xml`
at line 379, The string resource logout_revocation_note contains English menu
labels ("GitHub Settings > Applications") inside the Polish file; update the
value of string name="logout_revocation_note" to use Polish localized menu text
(e.g., "Ustawienia GitHub > Aplikacje" or a Polish appropriate localization such
as "Ustawienia GitHub → Aplikacje") so the entire sentence is in Polish and
consistent with values-pl resources.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…flow
This commit introduces a mechanism to detect and handle expired GitHub sessions and enhances the device authentication UI with countdown timers and better error categorization.
SessionExpiredDialogto notify users when their session is invalid or revoked.UnauthorizedInterceptor(401 handler) into the KtorHttpClientto automatically trigger session expiration events.SkipLogin(Continue as Guest) option to the authentication flow.AuthenticationViewModelwith specific recovery hints (e.g., connection issues, denied access).AuthenticationStateandTokenStoreto manage session expiration events and token lifecycle (includingsaved_attimestamp).clearAll()toCacheManagerto ensure all local data is purged upon logout or session expiration.LogoutDialogto include a note about revoking access via GitHub settings.Summary by CodeRabbit
New Features
Bug Fixes / Improvements