feat(auth): per-host PAT storage encrypted via KSafe#634
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds end-to-end host tokens management: domain types and contract, KSafe-backed HostTokenRepositoryImpl with validation, HostTokenInterceptor to inject Bearer headers, presentation state/viewmodel/actions/events, composable HostTokens UI and entry card, Koin wiring, and navigation integration from Tweaks. ChangesHost Tokens Feature Implementation
Sequence DiagramsequenceDiagram
participant UI
participant ViewModel
participant HostTokenRepository
participant KSafe
participant ValidationHttpClient
UI->>ViewModel: user adds/replaces token
ViewModel->>HostTokenRepository: set(host, token, displayName)
HostTokenRepository->>KSafe: persist JSON (host_tokens_v1)
ViewModel->>HostTokenRepository: validate(host, token)
HostTokenRepository->>ValidationHttpClient: GET /user with Bearer
ValidationHttpClient-->>HostTokenRepository: TokenValidation
HostTokenRepository-->>ViewModel: Result<TokenValidation>
ViewModel-->>UI: update state / show snackbar
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@core/data/src/commonMain/kotlin/zed/rainxch/core/data/network/interceptor/HostTokenInterceptor.kt`:
- Around line 10-16: Remove the non-essential KDoc block describing
HostTokenInterceptor behavior (the multiline comment lines 10–16) so the file no
longer contains that top-level documentation; locate the comment that references
HostTokenRepository and Authorization header and delete it, leaving the
HostTokenInterceptor class and any imports unchanged and ensuring no other
inline comments are added per repository policy.
- Around line 23-30: The interceptor currently blocks the request thread via
runBlocking when calling the suspend repo.get(host) inside the onRequest lambda
of HostTokenInterceptor; change the onRequest handler to call the suspend
repo.get(host) directly (i.e., remove runBlocking) and handle errors with
runCatching or try/catch so the token is obtained asynchronously (e.g., token =
runCatching { repo.get(host)?.token }.getOrNull() or equivalent), leaving the
rest of the logic that checks token and sets
request.header(HttpHeaders.Authorization, "Bearer $token") unchanged.
In
`@core/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/HostTokenRepositoryImpl.kt`:
- Line 41: HostTokenRepositoryImpl.observeAll() currently returns
cache.asStateFlow() without ensuring persisted tokens are loaded; change it to
trigger ensureLoaded() before emitting so subscribers after restart won't see an
empty list. Update observeAll() to call ensureLoaded() (or guard with an
ensured-loaded check) when the flow is first collected and then emit the cache
(e.g., ensureLoaded() before returning or use a flow that calls ensureLoaded()
and then emits cache.asStateFlow()); reference HostTokenRepositoryImpl,
observeAll(), ensureLoaded(), and cache to locate the change.
- Around line 135-139: persist currently swallows KSafe write failures by
calling runCatching { ksafe.put(KEY_TOKENS_JSON, encoded) } with no handling;
update HostTokenRepositoryImpl.persist to surface write errors instead of
ignoring them—either remove runCatching and let the exception propagate, or
catch the exception, log it (include KEY_TOKENS_JSON and encoded size/context)
and rethrow it (or return a failing Result) so callers know persistence failed;
ensure references to cache.value,
json.encodeToString(ListSerializer(HostToken.serializer()), list), and
ksafe.put(KEY_TOKENS_JSON, encoded) are preserved when implementing this change.
In
`@feature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/hosttokens/HostTokensViewModel.kt`:
- Around line 35-39: The repository call in HostTokensViewModel
(repository.observeAll() inside viewModelScope.launch) lacks local failure
handling; wrap the repository/Flow collection in a try/catch (or use Flow.catch)
so exceptions don’t cancel the coroutine and so you can update _state (e.g., set
isLoading = false and populate an error/message field) or emit a fallback empty
list on failure; apply the same pattern to every repository call in this class
(the launches around observeAll(), and the calls at the other mentioned
locations) so failures update state and provide user feedback instead of
crashing the coroutine.
- Around line 77-78: The OnValidate action handling currently calls
validateExisting(action.host) without guarding against concurrent validations,
causing shared UI flags (isValidating, pendingValidationFor) to be raced; update
validateExisting (and the similar validation function in lines 127-143) to first
check and early-return if isValidating is true or pendingValidationFor is
non-null (or if pendingValidationFor equals a different host), then atomically
set isValidating = true and pendingValidationFor = host at the start of the
routine, and in the finally block clear these flags only if pendingValidationFor
still equals the host (to avoid clearing flags set by a later validation);
ensure HostTokensAction.OnValidate handling remains a simple dispatch to
validateExisting after this guard.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c688677a-2287-4067-b725-e16051d63054
📒 Files selected for processing (20)
composeApp/src/commonMain/kotlin/zed/rainxch/githubstore/app/di/ViewModelsModule.ktcomposeApp/src/commonMain/kotlin/zed/rainxch/githubstore/app/navigation/AppNavigation.ktcomposeApp/src/commonMain/kotlin/zed/rainxch/githubstore/app/navigation/GithubStoreGraph.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/di/SharedModule.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/download/MultiSourceDownloaderImpl.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/network/interceptor/HostTokenInterceptor.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/HostTokenRepositoryImpl.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/InstalledAppsRepositoryImpl.ktcore/domain/src/commonMain/kotlin/zed/rainxch/core/domain/model/HostToken.ktcore/domain/src/commonMain/kotlin/zed/rainxch/core/domain/repository/HostTokenRepository.ktcore/presentation/src/commonMain/composeResources/values/strings.xmlcore/presentation/src/commonMain/kotlin/zed/rainxch/core/presentation/components/RepositoryCard.ktfeature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsViewModel.ktfeature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/TweaksRoot.ktfeature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/hosttokens/HostTokensAction.ktfeature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/hosttokens/HostTokensEntryCard.ktfeature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/hosttokens/HostTokensEvent.ktfeature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/hosttokens/HostTokensRoot.ktfeature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/hosttokens/HostTokensState.ktfeature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/hosttokens/HostTokensViewModel.kt
Greptile SummaryThis PR introduces per-host PAT storage encrypted via KSafe, a
Confidence Score: 4/5Safe to merge with the single-slot undo caveat noted in the prior review; no regressions to the existing OAuth path. The previous round of blockers (runBlocking deadlock, api.github.com never matching, recoverCatching swallowing cancellation, undo silently discarding data) have all been fixed. The remaining open item from the prior review — rapid multi-delete silently losing the first token — is a real data-loss path but already on the author's radar. The dead The single-slot Important Files Changed
Sequence DiagramsequenceDiagram
participant UI as HostTokensRoot
participant VM as HostTokensViewModel
participant Repo as HostTokenRepositoryImpl
participant KSafe as KSafe (encrypted)
participant HTTP as Ktor HttpClient
UI->>VM: onAction(OnAddConfirm)
VM->>Repo: set(host, token, displayName)
Repo->>Repo: "rmwLock.withLock { ensureLoadedLocked() }"
Repo->>KSafe: ksafe.put(KEY_TOKENS_JSON, encoded)
KSafe-->>Repo: success / throws
Repo->>Repo: "cache.value = newList"
Repo-->>VM: success
VM->>VM: "validateExisting(host, fromAdd=true)"
VM->>Repo: get(host)
Repo-->>VM: HostToken
VM->>Repo: validate(host, token)
Repo->>HTTP: GET /user (Authorization: Bearer …)
HTTP-->>Repo: HttpResponse
Repo-->>VM: Result TokenValidation
VM->>VM: update state (ValidationLine)
UI->>VM: onAction(OnDelete(host))
VM->>Repo: delete(host)
Repo->>KSafe: ksafe.put(updated list)
KSafe-->>Repo: success
Repo-->>VM: success
VM->>UI: event TokenDeletedWithUndo
UI->>VM: onAction(OnUndoDelete)
VM->>Repo: set(pending.host, pending.token, …)
Repo->>KSafe: ksafe.put(restored list)
KSafe-->>Repo: success / throws
Repo-->>VM: success / failure
VM->>UI: event Message (error, if failure)
Reviews (6): Last reviewed commit: "fix(host-tokens): undo failure surfaced,..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+405
to
412
| single<zed.rainxch.core.domain.repository.HostTokenRepository> { | ||
| zed.rainxch.core.data.repository.HostTokenRepositoryImpl( | ||
| ksafe = get(qualifier = org.koin.core.qualifier.named("tokens")), | ||
| httpClient = get(qualifier = org.koin.core.qualifier.named("test")), | ||
| ) | ||
| } | ||
|
|
||
| single<HttpClient>(qualifier = named("test")) { |
There was a problem hiding this comment.
named("test") is the Koin qualifier for the HttpClient injected into HostTokenRepositoryImpl and is also the binding used in production. The name "test" is misleading — it suggests a mock or test-only binding, which makes the DI graph harder to reason about and risks being removed or replaced by actual test infrastructure in the future. Consider renaming to something like named("validate") or named("plain").
Suggested change
| single<zed.rainxch.core.domain.repository.HostTokenRepository> { | |
| zed.rainxch.core.data.repository.HostTokenRepositoryImpl( | |
| ksafe = get(qualifier = org.koin.core.qualifier.named("tokens")), | |
| httpClient = get(qualifier = org.koin.core.qualifier.named("test")), | |
| ) | |
| } | |
| single<HttpClient>(qualifier = named("test")) { | |
| single<zed.rainxch.core.domain.repository.HostTokenRepository> { | |
| zed.rainxch.core.data.repository.HostTokenRepositoryImpl( | |
| ksafe = get(qualifier = org.koin.core.qualifier.named("tokens")), | |
| httpClient = get(qualifier = org.koin.core.qualifier.named("validate")), | |
| ) | |
| } | |
| single<HttpClient>(qualifier = named("validate")) { |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
…ng, ensureLoaded on observe
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
core/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/HostTokenRepositoryImpl.kt (2)
121-125: ⚡ Quick winRegex-based JSON parsing is fragile.
Using a regex to extract
loginfrom JSON can fail on edge cases (escaped quotes, unicode, whitespace variations). Since you already have aJsoninstance configured at line 43, consider defining a small data class and using proper deserialization:`@Serializable` private data class UserResponse(val login: String? = null)Then parse with
json.decodeFromString<UserResponse>(text).login.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/HostTokenRepositoryImpl.kt` around lines 121 - 125, Replace the fragile regex extraction for the login variable with proper JSON deserialization: define a private `@Serializable` data class (e.g., UserResponse with val login: String? = null), import kotlinx.serialization.Serializable, then in the runCatching block read response.bodyAsText() and call json.decodeFromString<UserResponse>(text).login to set login (reference the existing json instance at line 43 and the login variable assignment), removing the Regex handling.
36-36: 💤 Low value
initScopeis never cancelled, creating a potential resource leak.The
CoroutineScopecreated here has no lifecycle management. While the fire-and-forgetlaunchininitwill complete quickly, the scope itself persists for the lifetime of the object. IfHostTokenRepositoryImplis ever recreated (e.g., during testing or scope changes), the abandoned scope may linger.Consider using a scope tied to the application lifecycle or cancelling it when no longer needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@core/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/HostTokenRepositoryImpl.kt`:
- Around line 121-125: Replace the fragile regex extraction for the login
variable with proper JSON deserialization: define a private `@Serializable` data
class (e.g., UserResponse with val login: String? = null), import
kotlinx.serialization.Serializable, then in the runCatching block read
response.bodyAsText() and call json.decodeFromString<UserResponse>(text).login
to set login (reference the existing json instance at line 43 and the login
variable assignment), removing the Regex handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c00a510d-9865-4b66-8ba3-ae6412848b29
📒 Files selected for processing (4)
core/data/src/commonMain/kotlin/zed/rainxch/core/data/network/interceptor/HostTokenInterceptor.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/HostTokenRepositoryImpl.ktcore/presentation/src/commonMain/kotlin/zed/rainxch/core/presentation/components/RepositoryCard.ktfeature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/hosttokens/HostTokensViewModel.kt
✅ Files skipped from review due to trivial changes (1)
- core/presentation/src/commonMain/kotlin/zed/rainxch/core/presentation/components/RepositoryCard.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- feature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/hosttokens/HostTokensViewModel.kt
…idate, undo, replace/edit menu, OAuth coexistence note
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
feature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/hosttokens/HostTokensViewModel.kt (1)
36-49:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle
observeAll()/ auth stream failures locally.If either collect throws, the coroutine is cancelled and UI recovery/feedback is skipped. Wrap with
catch/runCatchingand update state on failure.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@feature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/hosttokens/HostTokensViewModel.kt` around lines 36 - 49, The two launched collectors (viewModelScope.launch { repository.observeAll().collect { ... } } and viewModelScope.launch { authenticationState.isUserLoggedIn().collect { ... } }) must handle upstream errors locally so the coroutine doesn't silently cancel the UI flow; wrap each flow collection with a try/catch or Flow.catch (or use runCatching around collect) and on failure call _state.update to set a failure state (e.g., isLoading = false and an error flag/message) so the UI can show feedback and continue; update the blocks that reference repository.observeAll(), authenticationState.isUserLoggedIn(), and _state.update accordingly to record errors and stop loading when exceptions occur.
🧹 Nitpick comments (4)
feature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/hosttokens/HostTokensEvent.kt (1)
8-12: ⚡ Quick winRemove KDoc comments from simple event cases.
These aren’t documenting invariants/workarounds, so they should be removed for guideline compliance.
As per coding guidelines, "Do not add KDoc or inline comments unless explicitly requested; only add inline comments for non-obvious invariants, tricky concurrency, or workarounds".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@feature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/hosttokens/HostTokensEvent.kt` around lines 8 - 12, Remove the non-essential KDoc comments attached to the simple event data classes in HostTokensEvent: delete the /** Snackbar with undo when a token is removed. */ comment above TokenDeletedWithUndo and the /** Open URL in browser (token creation pages on each forge). */ comment above OpenUrl in HostTokensEvent.kt so the file contains only the data class declarations (TokenDeletedWithUndo and OpenUrl) without KDoc; leave any other meaningful docs or comments for non-obvious behavior intact.feature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/hosttokens/HostTokensState.kt (1)
9-13: ⚡ Quick winDrop inline/KDoc comments in this state model.
These comments are not documenting tricky invariants/workarounds and should be removed to align with repo rules.
As per coding guidelines, "Do not add KDoc or inline comments unless explicitly requested; only add inline comments for non-obvious invariants, tricky concurrency, or workarounds".
Also applies to: 32-36
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@feature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/hosttokens/HostTokensState.kt` around lines 9 - 13, Remove the non-essential inline/KDoc comments in the HostTokensState data model: delete the comment lines above the properties validationByHost and validatingHosts so the file only contains the property declarations (val validationByHost: Map<String, ValidationLine> = emptyMap(), val validatingHosts: Set<String> = emptySet()). Also remove the similar non-essential comments around the other properties referenced in the review (lines 32-36) so the class conforms to the repo guideline of only keeping comments for non-obvious invariants, concurrency issues, or workarounds.core/domain/src/commonMain/kotlin/zed/rainxch/core/domain/model/HostToken.kt (1)
52-63: ⚡ Quick winRemove or move the misplaced KDoc block.
This block documents API-host collapsing but sits above
sanitizePastedToken, so it is misleading and should be removed or moved toapiHostToTokenHost.As per coding guidelines, "Do not add KDoc or inline comments unless explicitly requested; only add inline comments for non-obvious invariants, tricky concurrency, or workarounds".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/domain/src/commonMain/kotlin/zed/rainxch/core/domain/model/HostToken.kt` around lines 52 - 63, The KDoc describing API-host collapsing is misplaced above sanitizePastedToken and should be removed or relocated; move that block to immediately above the apiHostToTokenHost function (or delete it if redundant) so documentation correctly describes apiHostToTokenHost, and ensure sanitizePastedToken has only relevant docs or none per guidelines; update comments accordingly around the functions sanitizePastedToken and apiHostToTokenHost in HostToken.kt.feature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/hosttokens/HostTokensAction.kt (1)
9-9: ⚡ Quick winRemove section comments from action declarations.
These comments are non-essential and should be removed per repository Kotlin comment policy.
As per coding guidelines, "Do not add KDoc or inline comments unless explicitly requested; only add inline comments for non-obvious invariants, tricky concurrency, or workarounds".
Also applies to: 18-18, 24-24
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@feature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/hosttokens/HostTokensAction.kt` at line 9, Remove the non-essential section comments (e.g., the "// Picker / dialog lifecycle" and the comments at the other mentioned locations) from the action declarations in HostTokensAction (the action classes/objects declared in HostTokensAction.kt); simply delete these inline section comments so the file follows the Kotlin comment policy and leaves only necessary code and any truly essential comments around tricky invariants.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@feature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/hosttokens/HostTokensViewModel.kt`:
- Line 263: Replace the hardcoded fallback "validation failed" used when
building the validation result in HostTokensViewModel (the expression t.message
?: "validation failed") with the app's localized string resource; keep using
t.message when non-null but otherwise fetch the appropriate i18n string (e.g., a
ValidationFailed string from your shared resources / HostTokens strings
provider) and assign that to errorMessage so the fallback is localized.
- Around line 237-240: In undoDelete(), don't ignore
repository.set(pending.host, pending.token, pending.displayName): wrap the call
in runCatching and check the result like persistDraft() and deleteHost() do;
onFailure emit the same error event (so the UI can show the failure) and only
call _state.update { it.copy(pendingUndoDelete = null) } after a successful
result (onSuccess). Locate undoDelete(), the repository.set(...) invocation, and
the _state.update call to make this change.
- Around line 244-248: The guard in validateExisting is race-prone because it
checks state.value.validatingHosts before launching the coroutine; fix by making
the check-and-add atomic: introduce a private Mutex (e.g., validatingHostsMutex)
and inside the coroutine wrap the test-and-insert in
validatingHostsMutex.withLock { if (host in state.value.validatingHosts)
return@launch; _state.update { it.copy(validatingHosts = it.validatingHosts +
host) } }, then continue to call repository.validate(host, token) and remove the
host from _state.validatingHosts after completion; reference validateExisting,
viewModelScope.launch, _state.update, repository.validate, and
validatingHostsMutex.
---
Duplicate comments:
In
`@feature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/hosttokens/HostTokensViewModel.kt`:
- Around line 36-49: The two launched collectors (viewModelScope.launch {
repository.observeAll().collect { ... } } and viewModelScope.launch {
authenticationState.isUserLoggedIn().collect { ... } }) must handle upstream
errors locally so the coroutine doesn't silently cancel the UI flow; wrap each
flow collection with a try/catch or Flow.catch (or use runCatching around
collect) and on failure call _state.update to set a failure state (e.g.,
isLoading = false and an error flag/message) so the UI can show feedback and
continue; update the blocks that reference repository.observeAll(),
authenticationState.isUserLoggedIn(), and _state.update accordingly to record
errors and stop loading when exceptions occur.
---
Nitpick comments:
In
`@core/domain/src/commonMain/kotlin/zed/rainxch/core/domain/model/HostToken.kt`:
- Around line 52-63: The KDoc describing API-host collapsing is misplaced above
sanitizePastedToken and should be removed or relocated; move that block to
immediately above the apiHostToTokenHost function (or delete it if redundant) so
documentation correctly describes apiHostToTokenHost, and ensure
sanitizePastedToken has only relevant docs or none per guidelines; update
comments accordingly around the functions sanitizePastedToken and
apiHostToTokenHost in HostToken.kt.
In
`@feature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/hosttokens/HostTokensAction.kt`:
- Line 9: Remove the non-essential section comments (e.g., the "// Picker /
dialog lifecycle" and the comments at the other mentioned locations) from the
action declarations in HostTokensAction (the action classes/objects declared in
HostTokensAction.kt); simply delete these inline section comments so the file
follows the Kotlin comment policy and leaves only necessary code and any truly
essential comments around tricky invariants.
In
`@feature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/hosttokens/HostTokensEvent.kt`:
- Around line 8-12: Remove the non-essential KDoc comments attached to the
simple event data classes in HostTokensEvent: delete the /** Snackbar with undo
when a token is removed. */ comment above TokenDeletedWithUndo and the /** Open
URL in browser (token creation pages on each forge). */ comment above OpenUrl in
HostTokensEvent.kt so the file contains only the data class declarations
(TokenDeletedWithUndo and OpenUrl) without KDoc; leave any other meaningful docs
or comments for non-obvious behavior intact.
In
`@feature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/hosttokens/HostTokensState.kt`:
- Around line 9-13: Remove the non-essential inline/KDoc comments in the
HostTokensState data model: delete the comment lines above the properties
validationByHost and validatingHosts so the file only contains the property
declarations (val validationByHost: Map<String, ValidationLine> = emptyMap(),
val validatingHosts: Set<String> = emptySet()). Also remove the similar
non-essential comments around the other properties referenced in the review
(lines 32-36) so the class conforms to the repo guideline of only keeping
comments for non-obvious invariants, concurrency issues, or workarounds.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ae9e2a90-b2b4-4242-af71-155d0c3e7c1c
📒 Files selected for processing (7)
core/domain/src/commonMain/kotlin/zed/rainxch/core/domain/model/HostToken.ktcore/presentation/src/commonMain/composeResources/values/strings.xmlfeature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/hosttokens/HostTokensAction.ktfeature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/hosttokens/HostTokensEvent.ktfeature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/hosttokens/HostTokensRoot.ktfeature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/hosttokens/HostTokensState.ktfeature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/hosttokens/HostTokensViewModel.kt
Comment on lines
+244
to
+248
| if (host in state.value.validatingHosts) return | ||
| viewModelScope.launch { | ||
| val token = runCatching { repository.get(host)?.token }.getOrNull() ?: return@launch | ||
| _state.update { it.copy(validatingHosts = it.validatingHosts + host) } | ||
| val result = repository.validate(host, token) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n feature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/hosttokens/HostTokensViewModel.kt | sed -n '240,260p'Repository: OpenHub-Store/GitHub-Store
Length of output: 1088
🏁 Script executed:
rg "validateExisting" feature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/hosttokens/HostTokensViewModel.kt -B 2 -A 2Repository: OpenHub-Store/GitHub-Store
Length of output: 738
🏁 Script executed:
cat -n feature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/hosttokens/HostTokensViewModel.kt | sed -n '1,50p'Repository: OpenHub-Store/GitHub-Store
Length of output: 2537
validateExisting in-flight guard is race-prone.
The guard checks state.value.validatingHosts at line 244 before launching the coroutine, but the state update happens later inside the coroutine at line 247. This creates a timing window where multiple rapid calls can pass the guard check before any update the state, leading to duplicate validations for the same host.
Suggested fix
private fun validateExisting(host: String, fromAdd: Boolean = false) {
- if (host in state.value.validatingHosts) return
- viewModelScope.launch {
+ var shouldStart = false
+ _state.update {
+ if (host in it.validatingHosts) it
+ else {
+ shouldStart = true
+ it.copy(validatingHosts = it.validatingHosts + host)
+ }
+ }
+ if (!shouldStart) return
+ viewModelScope.launch {
val token = runCatching { repository.get(host)?.token }.getOrNull() ?: return@launch
- _state.update { it.copy(validatingHosts = it.validatingHosts + host) }
val result = repository.validate(host, token)
...
}
}📝 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
| if (host in state.value.validatingHosts) return | |
| viewModelScope.launch { | |
| val token = runCatching { repository.get(host)?.token }.getOrNull() ?: return@launch | |
| _state.update { it.copy(validatingHosts = it.validatingHosts + host) } | |
| val result = repository.validate(host, token) | |
| private fun validateExisting(host: String, fromAdd: Boolean = false) { | |
| var shouldStart = false | |
| _state.update { | |
| if (host in it.validatingHosts) it | |
| else { | |
| shouldStart = true | |
| it.copy(validatingHosts = it.validatingHosts + host) | |
| } | |
| } | |
| if (!shouldStart) return | |
| viewModelScope.launch { | |
| val token = runCatching { repository.get(host)?.token }.getOrNull() ?: return@launch | |
| val result = repository.validate(host, token) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@feature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/hosttokens/HostTokensViewModel.kt`
around lines 244 - 248, The guard in validateExisting is race-prone because it
checks state.value.validatingHosts before launching the coroutine; fix by making
the check-and-add atomic: introduce a private Mutex (e.g., validatingHostsMutex)
and inside the coroutine wrap the test-and-insert in
validatingHostsMutex.withLock { if (host in state.value.validatingHosts)
return@launch; _state.update { it.copy(validatingHosts = it.validatingHosts +
host) } }, then continue to call repository.validate(host, token) and remove the
host from _state.validatingHosts after completion; reference validateExisting,
viewModelScope.launch, _state.update, repository.validate, and
validatingHostsMutex.
…nit flow .catch, buffered events channel
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.
Summary
HostTokenRepository+ KSafe-backed impl (per-host PAT vault inside existingghs_tokensvault).HostTokenssub-screen under Tweaks with add / validate / delete + dialog.HostTokenInterceptorKtor plugin (opt-in, not wired into existing GitHub client yet).Test plan
Notes
feat/e8-codeberg-forgejo; rebase or merge order is up to you.Summary by CodeRabbit
New Features
Bug Fixes