Claude mds#263
Merged
Merged
Conversation
This commit introduces support for HTTP and SOCKS proxies in the application's network layer. It allows the `HttpClient` to be dynamically reconfigured at runtime when proxy settings change. The implementation creates a `GitHubClientProvider` that observes a `ProxyConfig` flow. When the proxy configuration is updated, it closes the existing Ktor `HttpClient` and creates a new one with the updated proxy settings. Platform-specific Ktor engines are used for proxy implementation: `OkHttp` on Android and `CIO` on JVM (desktop). - **feat(network)**: Added `ProxyConfig` data class to model proxy settings. - **feat(network)**: Introduced `ProxyManager` to hold and update the global proxy configuration. - **feat(network)**: Implemented `GitHubClientProvider` to manage the lifecycle of `HttpClient` and recreate it when proxy settings change. - **feat(network)**: Added platform-specific `HttpClientFactory` implementations for Android (`OkHttp`) and JVM (`CIO`) to handle proxy configuration. - **chore(deps)**: Added Ktor client engine dependencies: `ktor-client-okhttp` for Android and `ktor-client-cio` for JVM.
…ile UI This commit refactors the "Settings" feature into "Profile", including renaming repositories, data models, and UI components. It also introduces a new user profile section in the UI and updates translations across multiple languages. - **refactor**: Renamed `SettingsRepository` to `ProfileRepository` and updated its implementation and DI modules. - **feat(profile)**: Added `UserProfile` domain model and `accountSection` to display user information (avatar, etc.) in `ProfileRoot`. - **feat(profile)**: Reorganized `ProfileRoot` into modular sections: `profile`, `settings`, `about`, and `logout`. - **feat(i18n)**: Renamed `settings_title` to `profile_title` and updated translations for `ru`, `tr`, `bn`, `zh-rCN`, `es`, `fr`, `it`, `hi`, `ja`, `kr`, and `pl`. - **refactor(presentation)**: Created `GitHubStoreImage` component to unify image loading logic using Coil and shared it across `RepositoryCard` and profile sections. - **ui**: Added `SectionHeader` and `SectionTitle` components for consistent layout in the profile screen.
Contributor
WalkthroughAdds proxy configuration model and a ProxyManager StateFlow, platform-specific HttpClient factories (Android OkHttp, JVM CIO), a GitHubClientProvider that recreates HttpClient on proxy changes, DI registration for the provider, dependency updates for Ktor engines, and many CLAUDE.md docs for project and features. Changes
Sequence DiagramsequenceDiagram
participant PM as ProxyManager
participant GCP as GitHubClientProvider
participant HCF as HttpClientFactory
participant Engine as Platform\n(OkHttp/CIO)
participant Client as HttpClient
Note over PM,Client: Proxy Configuration Flow
PM->>PM: setProxyConfig(config)
PM->>GCP: currentProxyConfig emits ProxyConfig
activate GCP
GCP->>GCP: close() previous client
GCP->>HCF: createGitHubHttpClient(tokenStore,\nrateLimitRepository, newConfig)
activate HCF
HCF->>HCF: createPlatformHttpClient(newConfig)
activate Engine
Engine->>Engine: Configure proxy\n(type, host, port, auth)
Engine->>Client: Build HttpClient
deactivate Engine
HCF->>Client: Configure interceptors\n(token, rate-limit)
deactivate HCF
GCP->>GCP: Store new client\nin StateFlow
GCP->>GCP: Emit via client flow
deactivate GCP
Note over PM,Client: Client Usage
rect rgba(0, 200, 100, 0.5)
GCP->>GCP: currentClient() returns\nactive HttpClient
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/data/src/commonMain/kotlin/zed/rainxch/core/data/di/SharedModule.kt (1)
97-111:⚠️ Potential issue | 🟠 MajorSeven repositories silently bypass proxy configuration — static
HttpClientbinding ignoresGitHubClientProvider.
networkModulebinds bothsingle<GitHubClientProvider>(proxy-aware, reactive toProxyManagerchanges) andsingle<HttpClient>(created viacreateGitHubHttpClientwithout proxy argument). All seven repositories injected withhttpClient = get()receive only the static client:
- Core:
InstalledAppsRepositoryImpl(line 68),StarredRepositoryImpl(line 77)- Features:
SearchRepositoryImpl,DeveloperProfileRepositoryImpl,DetailsRepositoryImpl,HomeRepositoryImpl,AppsRepositoryImplThis means proxy configuration changes are never applied to their network calls.
The
HttpClientbinding should delegate toGitHubClientProvider.currentClient()to inherit proxy awareness:🔧 Option: align the bare HttpClient binding
- single<HttpClient> { - createGitHubHttpClient( - tokenStore = get(), - rateLimitRepository = get() - ) - } + single<HttpClient> { + get<GitHubClientProvider>().currentClient() + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/data/src/commonMain/kotlin/zed/rainxch/core/data/di/SharedModule.kt` around lines 97 - 111, networkModule currently registers a static single<HttpClient> using createGitHubHttpClient which ignores proxy updates and causes repositories that call get<HttpClient>() to bypass the proxy-aware GitHubClientProvider; change the HttpClient binding to delegate to the provider so it returns GitHubClientProvider.currentClient() (or call a provider method that yields the live client) instead of directly calling createGitHubHttpClient, ensuring ProxyManager.currentProxyConfig-driven client swaps propagate to all consumers like InstalledAppsRepositoryImpl, StarredRepositoryImpl and the various *RepositoryImpl classes.
🧹 Nitpick comments (3)
core/domain/src/commonMain/kotlin/zed/rainxch/core/domain/model/ProxyConfig.kt (1)
3-9: Validate proxy host/port at construction time.Fail-fast checks here prevent invalid configs from propagating into engine setup paths.
Proposed refactor
data class ProxyConfig( val type: ProxyType, val host: String, val port: Int, val username: String? = null, val password: String? = null, ) { + init { + require(host.isNotBlank()) { "Proxy host must not be blank" } + require(port in 1..65535) { "Proxy port must be between 1 and 65535" } + } + enum class ProxyType { HTTP, SOCKS } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/domain/src/commonMain/kotlin/zed/rainxch/core/domain/model/ProxyConfig.kt` around lines 3 - 9, Add fail-fast validation to ProxyConfig by adding an init block in the data class (ProxyConfig) that checks the host and port: ensure host is non-blank (trimmed) and a plausible host string (reject empty/whitespace) and ensure port is within 1..65535; for invalid values throw IllegalArgumentException with a clear message referencing the offending value. Implement this validation inside ProxyConfig's primary constructor/init so invalid ProxyConfig instances cannot be created and will surface immediately during construction.feature/apps/CLAUDE.md (1)
28-32: Remove redundantsuspendmodifier fromgetApps().A function returning
Flowis already cold/lazy and doesn't needsuspend— the flow body runs only upon collection. Combining both is a Kotlin antipattern that makes the interface harder to use (callers need a coroutine scope just to obtain theFlowreference). RemovesuspendfromgetApps()on line 8.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/apps/CLAUDE.md` around lines 28 - 32, Remove the unnecessary suspend modifier from the AppsRepository.getApps() declaration: change the signature in the AppsRepository interface to fun getApps(): Flow<List<InstalledApp>> and update all implementations/overrides to match (remove suspend from their getApps implementations) and any call sites that currently call getApps() from a coroutine just to obtain the Flow so they can directly use the returned Flow (or collect it in a coroutine). Ensure the method still returns a cold Flow and compile-check all classes implementing AppsRepository after adjusting the signatures.CLAUDE.md (1)
99-99: Use directory-level DI guidance instead of a hardcoded filename.The docs currently prescribe
data/di/SharedModule.kt; this can be misleading if modules are split across files. Preferdata/di/directory wording.As per coding guidelines, "Use Koin for dependency injection; define modules in each feature's
data/di/directory and register incomposeApp/.../app/di/initKoin.kt".Also applies to: 147-147
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLAUDE.md` at line 99, The documentation currently references a hardcoded module filename `data/di/SharedModule.kt`; update the wording to recommend using a directory-level approach by saying "define modules in each feature's `data/di/` directory" and keep the registration reference to `initKoin.kt` and usage `koinViewModel()` intact; also replace the second occurrence at the other mention (line noted in the review) so both spots use `data/di/` instead of the specific filename.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CLAUDE.md`:
- Around line 88-93: Add a language tag to the fenced code block in CLAUDE.md
that contains the list of screens (the block with HomeScreen, SearchScreen,
AuthenticationScreen, SettingsScreen, FavouritesScreen, StarredReposScreen,
AppsScreen and the lines DetailsScreen(repositoryId, owner, repo) and
DeveloperProfileScreen(username)); change the opening fence from ``` to
something like ```text (or ```txt) so markdownlint MD040 is satisfied and the
block is explicitly annotated.
In
`@core/data/src/androidMain/kotlin/zed/rainxch/core/data/network/HttpClientFactory.android.kt`:
- Around line 20-33: The proxyAuthenticator lambda in
HttpClientFactory.android.kt must guard against infinite retries by returning
null when the outgoing request already contains a "Proxy-Authorization" header;
update the proxyAuthenticator { _, response -> ... } block to check
response.request.header("Proxy-Authorization") and return null if present,
otherwise build and return the authenticated request. Also replace the
unnecessary force-unwrap config.username!! with the smart-cast config.username
(inside the existing if (config.username != null) block) and keep
config.password.orEmpty() when building Credentials.basic.
In
`@core/data/src/commonMain/kotlin/zed/rainxch/core/data/network/GitHubClientProvider.kt`:
- Around line 22-41: The client StateFlow creates an untracked initial
HttpClient and an ephemeral CoroutineScope, causing leaked scope and possible
reuse of a closed client; change the initialization to (1) create the initial
client via createGitHubHttpClient and assign it to _client.value, (2) store the
CoroutineScope(SupervisorJob() + Dispatchers.Default) in a member (e.g.,
providerScope) so it can be canceled, (3) when replacing a client in the map
block call oldClient.close(), set _client.value = null before/after closing and
cancel any per-client scope if used, and (4) ensure any public close/dispose
method cancels providerScope and closes+nulls _client.value to prevent returning
a closed HttpClient (update references to _client, client,
createGitHubHttpClient, and the scope creation accordingly).
In
`@core/data/src/jvmMain/kotlin/zed/rainxch/core/data/network/HttpClientFactory.jvm.kt`:
- Around line 9-23: The JVM CIO HttpClient created in createPlatformHttpClient
currently sets proxy via ProxyBuilder but lacks proxy authentication; update
createPlatformHttpClient to detect ProxyConfig.username/password and, when
present, add a Proxy-Authorization header (Basic <base64>) on outgoing proxy
requests by configuring defaultRequest on the HttpClient builder (alongside the
existing engine { proxy = ... } block) so HTTP proxies authenticate similarly to
the Android proxyAuthenticator; reference ProxyConfig, ProxyBuilder,
HttpClient(CIO), engine, and defaultRequest when making the change.
- Around line 17-19: The CIO engine does not support SOCKS so the current
ProxyConfig.ProxyType.SOCKS branch using ProxyBuilder.socks(...) will silently
be ignored; update HttpClientFactory (the JVM-specific client construction) to
detect ProxyConfig.ProxyType.SOCKS and fail fast: either throw a clear
UnsupportedOperationException/IllegalArgumentException with guidance to switch
to the OkHttp engine or an external SOCKS tunnel, or implement an alternative
JVM-only path that constructs an OkHttp-based client when ProxyType.SOCKS is
requested; replace the ProxyBuilder.socks(...) usage in the SOCKS branch with
this explicit fail-or-switch behavior so users are not silently unproxied.
In
`@core/domain/src/commonMain/kotlin/zed/rainxch/core/domain/model/ProxyConfig.kt`:
- Around line 3-10: ProxyConfig is a data class so its autogenerated toString()
will include username and password; override toString() in the ProxyConfig class
to redact or omit sensitive fields (username, password) while keeping type,
host, and port visible. Implement a custom toString() in ProxyConfig that
returns a string like "ProxyConfig(type=..., host=..., port=...,
username=REDACTED, password=REDACTED)" (or omits the fields when null) so logs
won't leak credentials; keep the class as a data class so copy/component
functions remain unchanged.
In `@feature/apps/CLAUDE.md`:
- Line 9: The fenced code block in feature/apps/CLAUDE.md is missing a language
specifier (MD040); update the opening fence from ``` to a specific language such
as ```text (or ```bash/```md as appropriate) for the block that begins with
"feature/apps/" so the lint rule is satisfied and the block is properly
highlighted.
In `@feature/auth/CLAUDE.md`:
- Line 9: Add a language specifier to the fenced code block in the
module-structure section of the CLAUDE.md file to fix markdownlint MD040; update
the opening fence from ``` to ```text (or ```plaintext) so the block that lists
the feature/auth directory structure is annotated, leaving the rest of the block
unchanged.
In `@feature/details/CLAUDE.md`:
- Line 72: Rewrite the fragment into a full, grammatically complete sentence:
replace the two fragments "Can be reached via repo ID or owner+name (for deep
links)." and "Falls back to owner+name lookup if `repositoryId == -1`." with a
single sentence such as "The repository can be reached via repository ID or
owner+name (for deep links), and falls back to owner+name lookup if
`repositoryId == -1`." Ensure `repositoryId == -1` remains code-formatted and
that the sentence reads smoothly in the surrounding prose.
In `@feature/dev-profile/CLAUDE.md`:
- Around line 9-30: The fenced module-structure block in feature/dev-profile/ is
missing a language tag (triggering MD040); update the markdown fence that begins
the tree block so it includes a language identifier (e.g., "text") immediately
after the opening triple backticks, leaving the block content unchanged—locate
the fenced block in CLAUDE.md that contains the directory tree and add the
language annotation to the opening ``` fence.
In `@feature/favourites/CLAUDE.md`:
- Line 9: The fenced code block in feature/favourites/CLAUDE.md is missing a
language specifier which triggers MD040; update the triple-backtick fence to
include a language (e.g., add "text" after the opening ```), so the block
becomes ```text before the directory snippet and preserves the subsequent lines;
ensure the opening fence exactly matches the suggested language specifier to
satisfy the linter.
In `@feature/home/CLAUDE.md`:
- Line 9: The fenced code block that begins with ``` and contains the directory
tree starting with "feature/home/" must include a language specifier to satisfy
MD040; update the opening fence (```) to include a language token (e.g., change
``` to ```text) so the block showing "feature/home/ ├── domain/" and its
children is explicitly marked as plain text.
In `@feature/search/CLAUDE.md`:
- Around line 9-29: The fenced tree block in feature/search/CLAUDE.md is missing
a language tag which triggers markdownlint MD040; update the opening
triple-backtick for the directory tree (the fenced block that begins with ``` )
to include a language tag such as text (i.e., change ``` to ```text) so the tree
is treated as plain text; ensure only the opening fence is modified and no other
content in the block (the block showing feature/search/ and its subfolders) is
altered.
In `@feature/starred/CLAUDE.md`:
- Around line 9-20: Update the fenced code block in feature/starred/CLAUDE.md to
include a language identifier (e.g., ```text) to satisfy markdown lint MD040;
locate the directory listing block (the triple-backtick fenced block containing
the feature/starred/ presentation/ tree and filenames such as
StarredReposViewModel.kt, StarredReposState.kt, StarredReposAction.kt,
StarredReposRoot.kt, model/StarredRepositoryUi.kt,
mappers/StarredRepoToUiMapper.kt, utils/TimeFormatUtils.kt, and
components/StarredRepositoryItem.kt) and change the opening fence from ``` to
```text.
---
Outside diff comments:
In `@core/data/src/commonMain/kotlin/zed/rainxch/core/data/di/SharedModule.kt`:
- Around line 97-111: networkModule currently registers a static
single<HttpClient> using createGitHubHttpClient which ignores proxy updates and
causes repositories that call get<HttpClient>() to bypass the proxy-aware
GitHubClientProvider; change the HttpClient binding to delegate to the provider
so it returns GitHubClientProvider.currentClient() (or call a provider method
that yields the live client) instead of directly calling createGitHubHttpClient,
ensuring ProxyManager.currentProxyConfig-driven client swaps propagate to all
consumers like InstalledAppsRepositoryImpl, StarredRepositoryImpl and the
various *RepositoryImpl classes.
---
Nitpick comments:
In `@CLAUDE.md`:
- Line 99: The documentation currently references a hardcoded module filename
`data/di/SharedModule.kt`; update the wording to recommend using a
directory-level approach by saying "define modules in each feature's `data/di/`
directory" and keep the registration reference to `initKoin.kt` and usage
`koinViewModel()` intact; also replace the second occurrence at the other
mention (line noted in the review) so both spots use `data/di/` instead of the
specific filename.
In
`@core/domain/src/commonMain/kotlin/zed/rainxch/core/domain/model/ProxyConfig.kt`:
- Around line 3-9: Add fail-fast validation to ProxyConfig by adding an init
block in the data class (ProxyConfig) that checks the host and port: ensure host
is non-blank (trimmed) and a plausible host string (reject empty/whitespace) and
ensure port is within 1..65535; for invalid values throw
IllegalArgumentException with a clear message referencing the offending value.
Implement this validation inside ProxyConfig's primary constructor/init so
invalid ProxyConfig instances cannot be created and will surface immediately
during construction.
In `@feature/apps/CLAUDE.md`:
- Around line 28-32: Remove the unnecessary suspend modifier from the
AppsRepository.getApps() declaration: change the signature in the AppsRepository
interface to fun getApps(): Flow<List<InstalledApp>> and update all
implementations/overrides to match (remove suspend from their getApps
implementations) and any call sites that currently call getApps() from a
coroutine just to obtain the Flow so they can directly use the returned Flow (or
collect it in a coroutine). Ensure the method still returns a cold Flow and
compile-check all classes implementing AppsRepository after adjusting the
signatures.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
CLAUDE.mdcore/data/build.gradle.ktscore/data/src/androidMain/kotlin/zed/rainxch/core/data/network/HttpClientFactory.android.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/di/SharedModule.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/ProxyManager.ktcore/data/src/jvmMain/kotlin/zed/rainxch/core/data/network/HttpClientFactory.jvm.ktcore/domain/src/commonMain/kotlin/zed/rainxch/core/domain/model/ProxyConfig.ktfeature/apps/CLAUDE.mdfeature/auth/CLAUDE.mdfeature/details/CLAUDE.mdfeature/dev-profile/CLAUDE.mdfeature/favourites/CLAUDE.mdfeature/home/CLAUDE.mdfeature/search/CLAUDE.mdfeature/settings/CLAUDE.mdfeature/starred/CLAUDE.mdgradle/libs.versions.toml
Comment on lines
+88
to
+93
| ``` | ||
| HomeScreen, SearchScreen, AuthenticationScreen, SettingsScreen, | ||
| FavouritesScreen, StarredReposScreen, AppsScreen | ||
| DetailsScreen(repositoryId, owner, repo) | ||
| DeveloperProfileScreen(username) | ||
| ``` |
Contributor
There was a problem hiding this comment.
Add a language to the fenced code block.
The block starting at Line 88 should use a language tag (e.g., ```text) to satisfy markdownlint MD040.
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 88-88: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CLAUDE.md` around lines 88 - 93, Add a language tag to the fenced code block
in CLAUDE.md that contains the list of screens (the block with HomeScreen,
SearchScreen, AuthenticationScreen, SettingsScreen, FavouritesScreen,
StarredReposScreen, AppsScreen and the lines DetailsScreen(repositoryId, owner,
repo) and DeveloperProfileScreen(username)); change the opening fence from ```
to something like ```text (or ```txt) so markdownlint MD040 is satisfied and the
block is explicitly annotated.
Comment on lines
+20
to
+33
| if (config.username != null) { | ||
| config { | ||
| proxyAuthenticator { _, response -> | ||
| response.request.newBuilder() | ||
| .header( | ||
| "Proxy-Authorization", | ||
| Credentials.basic( | ||
| config.username!!, | ||
| config.password.orEmpty() | ||
| ) | ||
| ) | ||
| .build() | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
proxyAuthenticator never returns null — risks infinite retry on bad credentials.
OkHttp will keep calling the authenticator until it returns null (to abort) or a max-retry limit is hit. Returning a request unconditionally on every challenge means a misconfigured password will spin indefinitely. Add a guard that returns null when the header is already present.
Additionally, config.username!! on line 27 is unnecessary — Kotlin should smart-cast username to non-null inside the if (config.username != null) block; replacing !! with the smart-cast value improves readability.
🛡️ Proposed fix
proxyAuthenticator { _, response ->
+ // Abort if we've already attached credentials (wrong password)
+ if (response.request.header("Proxy-Authorization") != null) return@proxyAuthenticator null
response.request.newBuilder()
.header(
"Proxy-Authorization",
Credentials.basic(
- config.username!!,
+ config.username,
config.password.orEmpty()
)
)
.build()
}🤖 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/network/HttpClientFactory.android.kt`
around lines 20 - 33, The proxyAuthenticator lambda in
HttpClientFactory.android.kt must guard against infinite retries by returning
null when the outgoing request already contains a "Proxy-Authorization" header;
update the proxyAuthenticator { _, response -> ... } block to check
response.request.header("Proxy-Authorization") and return null if present,
otherwise build and return the authenticated request. Also replace the
unnecessary force-unwrap config.username!! with the smart-cast config.username
(inside the existing if (config.username != null) block) and keep
config.password.orEmpty() when building Credentials.basic.
Comment on lines
+9
to
+23
| actual fun createPlatformHttpClient(proxyConfig: ProxyConfig?): HttpClient { | ||
| return HttpClient(CIO) { | ||
| engine { | ||
| proxy = proxyConfig?.let { config -> | ||
| when (config.type) { | ||
| ProxyConfig.ProxyType.HTTP -> ProxyBuilder.http( | ||
| Url("http://${config.host}:${config.port}") | ||
| ) | ||
| ProxyConfig.ProxyType.SOCKS -> ProxyBuilder.socks( | ||
| config.host, config.port | ||
| ) | ||
| } | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
JVM implementation is missing proxy authentication support.
The Android counterpart (HttpClientFactory.android.kt) handles username/password via a proxyAuthenticator. The JVM/CIO implementation has no equivalent, so authenticated proxies will silently fail on Desktop. Ktor docs recommend appending a Proxy-Authorization header via defaultRequest for HTTP proxy authentication on JVM.
🔧 Suggested fix — add proxy auth via `defaultRequest`
import io.ktor.client.HttpClient
import io.ktor.client.engine.ProxyBuilder
import io.ktor.client.engine.cio.CIO
+import io.ktor.client.plugins.defaultRequest
+import io.ktor.http.HttpHeaders
import io.ktor.http.Url
+import java.util.Base64
import zed.rainxch.core.domain.model.ProxyConfig
actual fun createPlatformHttpClient(proxyConfig: ProxyConfig?): HttpClient {
return HttpClient(CIO) {
engine {
proxy = proxyConfig?.let { config ->
when (config.type) {
ProxyConfig.ProxyType.HTTP -> ProxyBuilder.http(
Url("http://${config.host}:${config.port}")
)
ProxyConfig.ProxyType.SOCKS -> ProxyBuilder.socks(
config.host, config.port
)
}
}
}
+ if (proxyConfig?.username != null) {
+ defaultRequest {
+ val credentials = Base64.getEncoder()
+ .encodeToString("${proxyConfig.username}:${proxyConfig.password.orEmpty()}".toByteArray())
+ header(HttpHeaders.ProxyAuthorization, "Basic $credentials")
+ }
+ }
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@core/data/src/jvmMain/kotlin/zed/rainxch/core/data/network/HttpClientFactory.jvm.kt`
around lines 9 - 23, The JVM CIO HttpClient created in createPlatformHttpClient
currently sets proxy via ProxyBuilder but lacks proxy authentication; update
createPlatformHttpClient to detect ProxyConfig.username/password and, when
present, add a Proxy-Authorization header (Basic <base64>) on outgoing proxy
requests by configuring defaultRequest on the HttpClient builder (alongside the
existing engine { proxy = ... } block) so HTTP proxies authenticate similarly to
the Android proxyAuthenticator; reference ProxyConfig, ProxyBuilder,
HttpClient(CIO), engine, and defaultRequest when making the change.
Comment on lines
+17
to
+19
| ProxyConfig.ProxyType.SOCKS -> ProxyBuilder.socks( | ||
| config.host, config.port | ||
| ) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Ktor 3.x CIO engine SOCKS proxy support
💡 Result:
Ktor 3.x CIO client engine does not support SOCKS proxies (it supports HTTP proxy only). The official Ktor proxy-support table lists CIO: HTTP ✅ / SOCKS ✖️. [1]
Even though ProxyBuilder.socks(host, port) exists in Ktor’s API, it won’t work with the CIO engine because the engine itself lacks SOCKS support. [1][2]
Workarounds / alternatives (Ktor 3.x):
- Switch to an engine that supports SOCKS, e.g. OkHttp (JVM/Android), Android engine, or Curl (Kotlin/Native) — all show SOCKS ✅. [1]
- If you must stay on CIO, you’ll need an external approach (e.g., local redirector/tunnel) since CIO won’t do SOCKS natively. [1]
Sources:
[1] Ktor “Proxy” docs (supported engines table)
[2] Ktor API: ProxyBuilder.socks(...)
SOCKS proxy is not supported by the CIO engine and will be silently ignored.
The Ktor 3.x CIO engine supports HTTP proxy only. The official Ktor documentation confirms that CIO is listed as: HTTP ✅ / SOCKS ✖️. Even though ProxyBuilder.socks() is a valid API, the underlying engine lacks actual SOCKS implementation, so this branch will silently degrade to no proxy instead of failing fast.
Workarounds: Switch to the OkHttp engine (available on JVM), or implement an external SOCKS tunnel/redirector.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@core/data/src/jvmMain/kotlin/zed/rainxch/core/data/network/HttpClientFactory.jvm.kt`
around lines 17 - 19, The CIO engine does not support SOCKS so the current
ProxyConfig.ProxyType.SOCKS branch using ProxyBuilder.socks(...) will silently
be ignored; update HttpClientFactory (the JVM-specific client construction) to
detect ProxyConfig.ProxyType.SOCKS and fail fast: either throw a clear
UnsupportedOperationException/IllegalArgumentException with guidance to switch
to the OkHttp engine or an external SOCKS tunnel, or implement an alternative
JVM-only path that constructs an OkHttp-based client when ProxyType.SOCKS is
requested; replace the ProxyBuilder.socks(...) usage in the SOCKS branch with
this explicit fail-or-switch behavior so users are not silently unproxied.
Comment on lines
+9
to
+30
| ``` | ||
| feature/dev-profile/ | ||
| ├── domain/ | ||
| │ ├── model/ | ||
| │ │ ├── DeveloperProfile.kt # User profile data model | ||
| │ │ ├── DeveloperRepository.kt # User's repository model | ||
| │ │ ├── RepoFilterType.kt # Filter: All, Sources, Forks, etc. | ||
| │ │ └── RepoSortType.kt # Sort: Stars, Name, Updated, etc. | ||
| │ └── repository/DeveloperProfileRepository.kt # Profile + repos | ||
| ├── data/ | ||
| │ ├── di/SharedModule.kt # Koin: devProfileModule | ||
| │ ├── repository/DeveloperProfileRepositoryImpl.kt | ||
| │ ├── dto/ # Network DTOs | ||
| │ └── mappers/ # DTO → domain model mappers | ||
| └── presentation/ | ||
| ├── DeveloperProfileViewModel.kt # Profile loading, repo filtering/sorting | ||
| ├── DeveloperProfileState.kt # profile, repos, filters, loading | ||
| ├── DeveloperProfileAction.kt # Load, filter, sort, click actions | ||
| ├── DeveloperProfileEvent.kt # One-off events | ||
| ├── DeveloperProfileRoot.kt # Main composable | ||
| └── components/ # Profile header, repo list, filter controls | ||
| ``` |
Contributor
There was a problem hiding this comment.
Add a language to the fenced module-structure block.
Markdown lint MD040 is triggered here; annotate the fence (e.g., text).
Proposed fix
-```
+```text
feature/dev-profile/
├── domain/
│ ├── model/
│ │ ├── DeveloperProfile.kt # User profile data model
@@
└── presentation/
├── DeveloperProfileViewModel.kt # Profile loading, repo filtering/sorting
├── DeveloperProfileState.kt # profile, repos, filters, loading
├── DeveloperProfileAction.kt # Load, filter, sort, click actions
├── DeveloperProfileEvent.kt # One-off events
├── DeveloperProfileRoot.kt # Main composable
└── components/ # Profile header, repo list, filter controls</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **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.
```suggestion
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 9-9: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@feature/dev-profile/CLAUDE.md` around lines 9 - 30, The fenced
module-structure block in feature/dev-profile/ is missing a language tag
(triggering MD040); update the markdown fence that begins the tree block so it
includes a language identifier (e.g., "text") immediately after the opening
triple backticks, leaving the block content unchanged—locate the fenced block in
CLAUDE.md that contains the directory tree and add the language annotation to
the opening ``` fence.
|
|
||
| ## Module Structure | ||
|
|
||
| ``` |
Contributor
There was a problem hiding this comment.
Add a language specifier to the fenced code block (MD040).
📝 Suggested fix
-```
+```text
feature/favourites/
└── presentation/📝 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
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 9-9: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@feature/favourites/CLAUDE.md` at line 9, The fenced code block in
feature/favourites/CLAUDE.md is missing a language specifier which triggers
MD040; update the triple-backtick fence to include a language (e.g., add "text"
after the opening ```), so the block becomes ```text before the directory
snippet and preserves the subsequent lines; ensure the opening fence exactly
matches the suggested language specifier to satisfy the linter.
|
|
||
| ## Module Structure | ||
|
|
||
| ``` |
Contributor
There was a problem hiding this comment.
Add a language specifier to the fenced code block (MD040).
📝 Suggested fix
-```
+```text
feature/home/
├── domain/🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 9-9: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@feature/home/CLAUDE.md` at line 9, The fenced code block that begins with ```
and contains the directory tree starting with "feature/home/" must include a
language specifier to satisfy MD040; update the opening fence (```) to include a
language token (e.g., change ``` to ```text) so the block showing "feature/home/
├── domain/" and its children is explicitly marked as plain text.
Comment on lines
+9
to
+29
| ``` | ||
| feature/search/ | ||
| ├── domain/ | ||
| │ ├── model/ | ||
| │ │ ├── SearchPlatform.kt # All, Android, Windows, macOS, Linux | ||
| │ │ ├── ProgrammingLanguage.kt # Language filter options | ||
| │ │ └── SortBy.kt # Sort options (stars, updated, etc.) | ||
| │ └── repository/SearchRepository.kt # Filtered, paginated search | ||
| ├── data/ | ||
| │ ├── di/SharedModule.kt # Koin: searchModule | ||
| │ ├── repository/SearchRepositoryImpl.kt # GitHub search API integration | ||
| │ ├── dto/ # Network DTOs | ||
| │ └── mappers/ # DTO → domain model mappers | ||
| └── presentation/ | ||
| ├── SearchViewModel.kt # Search state, filter management, pagination | ||
| ├── SearchState.kt # query, results, filters, loading state | ||
| ├── SearchAction.kt # Search, filter changes, load more, clicks | ||
| ├── SearchEvent.kt # One-off events | ||
| ├── SearchRoot.kt # Main composable with search bar + filter dropdowns | ||
| └── components/ # Filter UI components | ||
| ``` |
Contributor
There was a problem hiding this comment.
Specify a language for the fenced tree block.
This triggers markdownlint MD040; use a language tag like text.
Proposed fix
-```
+```text
feature/search/
├── domain/
│ ├── model/
│ │ ├── SearchPlatform.kt # All, Android, Windows, macOS, Linux
@@
└── presentation/
├── SearchViewModel.kt # Search state, filter management, pagination
├── SearchState.kt # query, results, filters, loading state
├── SearchAction.kt # Search, filter changes, load more, clicks
├── SearchEvent.kt # One-off events
├── SearchRoot.kt # Main composable with search bar + filter dropdowns
└── components/ # Filter UI components</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **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.
```suggestion
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 9-9: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@feature/search/CLAUDE.md` around lines 9 - 29, The fenced tree block in
feature/search/CLAUDE.md is missing a language tag which triggers markdownlint
MD040; update the opening triple-backtick for the directory tree (the fenced
block that begins with ``` ) to include a language tag such as text (i.e.,
change ``` to ```text) so the tree is treated as plain text; ensure only the
opening fence is modified and no other content in the block (the block showing
feature/search/ and its subfolders) is altered.
Comment on lines
+9
to
+20
| ``` | ||
| feature/starred/ | ||
| └── presentation/ | ||
| ├── StarredReposViewModel.kt # Observes starred repos, handles remove | ||
| ├── StarredReposState.kt # starred list, loading | ||
| ├── StarredReposAction.kt # RemoveStarred, click actions | ||
| ├── StarredReposRoot.kt # Main composable (list of starred repos) | ||
| ├── model/StarredRepositoryUi.kt # UI model for display | ||
| ├── mappers/StarredRepoToUiMapper.kt # Domain → UI model mapper | ||
| ├── utils/TimeFormatUtils.kt # Time formatting utilities | ||
| └── components/StarredRepositoryItem.kt # Individual starred repo card | ||
| ``` |
Contributor
There was a problem hiding this comment.
Add a language identifier to the fenced block.
Markdown lint MD040 is raised for this block.
Proposed fix
-```
+```text
feature/starred/
└── presentation/
├── StarredReposViewModel.kt # Observes starred repos, handles remove
├── StarredReposState.kt # starred list, loading
├── StarredReposAction.kt # RemoveStarred, click actions
├── StarredReposRoot.kt # Main composable (list of starred repos)
├── model/StarredRepositoryUi.kt # UI model for display
├── mappers/StarredRepoToUiMapper.kt # Domain → UI model mapper
├── utils/TimeFormatUtils.kt # Time formatting utilities
└── components/StarredRepositoryItem.kt # Individual starred repo card</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **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.
```suggestion
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 9-9: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@feature/starred/CLAUDE.md` around lines 9 - 20, Update the fenced code block
in feature/starred/CLAUDE.md to include a language identifier (e.g., ```text) to
satisfy markdown lint MD040; locate the directory listing block (the
triple-backtick fenced block containing the feature/starred/ presentation/ tree
and filenames such as StarredReposViewModel.kt, StarredReposState.kt,
StarredReposAction.kt, StarredReposRoot.kt, model/StarredRepositoryUi.kt,
mappers/StarredRepoToUiMapper.kt, utils/TimeFormatUtils.kt, and
components/StarredRepositoryItem.kt) and change the opening fence from ``` to
```text.
…proxy support This commit refines the proxy configuration model and enhances how the application handles network proxies across Android and JVM platforms. The previous `ProxyConfig` data class has been replaced with a sealed class hierarchy to explicitly support "None", "System", "HTTP", and "SOCKS" configurations. - **feat(domain)**: Refactored `ProxyConfig` into a sealed class with `None`, `System`, `Http`, and `Socks` subtypes. - **feat(data)**: Added support for automatic system proxy detection in `HttpClientFactory` for both Android (using `ProxySelector`) and JVM (via `java.net.ProxySelector`). - **feat(data)**: Improved `ProxyManager` with dedicated methods for setting different proxy types (`setNoProxy`, `setSystemProxy`, `setHttpProxy`, `setSocksProxy`). - **refactor(data)**: Updated `GitHubClientProvider` to use a more robust thread-safe mechanism (using `Mutex`) for recreating the `HttpClient` when proxy settings change. - **fix(android)**: Added support for SOCKS proxy authentication on Android using `java.net.Authenticator`. - **fix(jvm)**: Implemented manual system proxy resolution in the CIO engine by selecting the appropriate proxy for GitHub API URIs.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
core/data/src/jvmMain/kotlin/zed/rainxch/core/data/network/HttpClientFactory.jvm.kt (2)
36-38:⚠️ Potential issue | 🟠 MajorHTTP proxy authentication credentials are ignored on JVM.
The Android implementation applies
Proxy-AuthorizationviaproxyAuthenticator, but this JVM/CIO implementation discardsproxyConfig.usernameandproxyConfig.passwordentirely. Authenticated HTTP proxies will fail silently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/data/src/jvmMain/kotlin/zed/rainxch/core/data/network/HttpClientFactory.jvm.kt` around lines 36 - 38, The JVM/CIO branch ignores proxy credentials: when handling ProxyConfig.Http in HttpClientFactory.jvm.kt (the ProxyBuilder.http(...) block) capture proxyConfig.username and proxyConfig.password and set up proxy authentication using the client's proxyAuthenticator (or equivalent) so requests to authenticated HTTP proxies include a Proxy-Authorization header; update the ProxyBuilder.http branch to create a ProxyBuilder with the proxy URL and, if proxyConfig.username/password are present, configure a proxy authenticator that applies basic auth using those values (mirror the Android implementation's use of proxyAuthenticator while referencing ProxyConfig.Http, proxyConfig.username, proxyConfig.password, and ProxyBuilder.http).
40-42:⚠️ Potential issue | 🟠 MajorCIO engine does not support SOCKS proxies — this branch silently does nothing.
ProxyBuilder.socks()is a valid API call, but the CIO engine ignores it. Users configuring a SOCKS proxy on JVM/Desktop will silently connect without any proxy. Consider either switching to the OkHttp engine for JVM or throwing anUnsupportedOperationExceptionto fail fast.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/data/src/jvmMain/kotlin/zed/rainxch/core/data/network/HttpClientFactory.jvm.kt` around lines 40 - 42, The CIO engine branch silently ignores ProxyConfig.Socks (using ProxyBuilder.socks), so update HttpClientFactory to either (A) switch to the OkHttp engine on JVM when ProxyConfig.Socks is provided—construct the HttpClient with the OkHttp engine and configure its proxy using the SOCKS settings from ProxyConfig.Socks—or (B) fail fast by throwing an UnsupportedOperationException when encountering ProxyConfig.Socks in the JVM/CIO path; locate the branch handling ProxyConfig.Socks (where ProxyBuilder.socks is called) and implement one of these two fixes so SOCKS proxy configuration is honored or clearly rejected.core/domain/src/commonMain/kotlin/zed/rainxch/core/domain/model/ProxyConfig.kt (1)
8-20:HttpandSocksdata classes still leakpasswordviatoString().Both are
data classsubtypes, so their auto-generatedtoString()will includepasswordin plain text. OverridetoString()to redact credentials.Proposed fix
data class Http( val host: String, val port: Int, val username: String? = null, val password: String? = null, - ) : ProxyConfig() + ) : ProxyConfig() { + override fun toString(): String = + "Http(host=$host, port=$port, username=$username, password=<redacted>)" + } data class Socks( val host: String, val port: Int, val username: String? = null, val password: String? = null, - ) : ProxyConfig() + ) : ProxyConfig() { + override fun toString(): String = + "Socks(host=$host, port=$port, username=$username, password=<redacted>)" + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/domain/src/commonMain/kotlin/zed/rainxch/core/domain/model/ProxyConfig.kt` around lines 8 - 20, Http and Socks data classes leak credentials via the auto-generated toString(); override toString() in both classes (Http and Socks) to return a string that includes host, port and username but redacts password (e.g., show "<redacted>" or null) so plain-text passwords are not printed; implement the custom toString() in each data class (overriding toString()) instead of changing other behaviors so equals/hashCode remain the data class defaults.core/data/src/androidMain/kotlin/zed/rainxch/core/data/network/HttpClientFactory.android.kt (1)
31-45:proxyAuthenticatorrisks infinite retry on bad credentials, andusername!!is unnecessary.The authenticator unconditionally returns a request with credentials on every challenge. OkHttp will keep calling it until it returns
nullor a retry limit is reached. Also,proxyConfig.username!!on line 38 is a redundant force-unwrap inside theif (proxyConfig.username != null)guard.Proposed fix
config { proxyAuthenticator { _, response -> + if (response.request.header("Proxy-Authorization") != null) { + return@proxyAuthenticator null + } response.request.newBuilder() .header( "Proxy-Authorization", Credentials.basic( - proxyConfig.username!!, + proxyConfig.username, proxyConfig.password.orEmpty() ) ) .build() } }🤖 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/network/HttpClientFactory.android.kt` around lines 31 - 45, The proxyAuthenticator currently always returns a new request with credentials and force-unwraps proxyConfig.username, which can cause infinite retry loops and unsafe null usage; inside the proxyAuthenticator lambda (symbol: proxyAuthenticator) safely read username/password from proxyConfig (avoid proxyConfig.username!!), and short-circuit returning null when the request already has a "Proxy-Authorization" header or the response has been challenged before (e.g., inspect response.request.header("Proxy-Authorization") or count prior responses) so you only add Credentials.basic(proxyConfig.username, proxyConfig.password.orEmpty()) once and return null on subsequent challenges to prevent infinite retries.
🧹 Nitpick comments (4)
core/data/src/commonMain/kotlin/zed/rainxch/core/data/network/GitHubClientProvider.kt (1)
56-59:close()should guard against double-close and concurrent access.If
close()is called while a proxy change is mid-flight (insidemutex.withLock), or called multiple times, it could close a client that's already been replaced or is being used.Proposed improvement
fun close() { - currentClient?.close() - scope.cancel() + scope.cancel() // cancel first to stop new client creation + mutex.withLock { + currentClient?.close() + currentClient = null + } }Note: if adopting the restructured approach from the previous comment, adjust accordingly with
runBlockingfor the mutex or use a separateAtomicBooleanclosed flag.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/data/src/commonMain/kotlin/zed/rainxch/core/data/network/GitHubClientProvider.kt` around lines 56 - 59, Wrap close() to be safe against concurrent calls by guarding with a closed flag or the existing mutex: either (A) add an AtomicBoolean closed that you set true atomically at start of close() and return if already closed, then close currentClient, set currentClient = null and cancel scope; or (B) if you prefer to use the suspend mutex, acquire it from close() via runBlocking { mutex.withLock { if (closed) return; closed = true; currentClient?.close(); currentClient = null; scope.cancel() } } so you never close a replaced client or race with proxy-change logic in mutex.withLock. Ensure you reference and update currentClient, scope, mutex, and the new closed flag consistently.core/data/src/androidMain/kotlin/zed/rainxch/core/data/network/HttpClientFactory.android.kt (1)
12-73: Port validation is missing —InetSocketAddresswith port 0 or negative values may throw or silently misbehave.
InetSocketAddress(proxyConfig.host, proxyConfig.port)is called on lines 29 and 51 without validating port range. If invalid values propagate fromProxyManager, the error will surface here with an unhelpful stack trace.🤖 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/network/HttpClientFactory.android.kt` around lines 12 - 73, In createPlatformHttpClient, validate proxyConfig.port before constructing InetSocketAddress for ProxyConfig.Http and ProxyConfig.Socks: ensure port is within 1..65535 (reject 0 and negative values) and handle invalid ports by throwing a clear IllegalArgumentException (including the bad value and host) or by skipping proxy setup/falling back to Proxy.NO_PROXY; update the branches for ProxyConfig.Http and ProxyConfig.Socks (where InetSocketAddress(...) is used) to perform this check and fail-fast with a descriptive error to avoid silent misbehavior.core/data/src/commonMain/kotlin/zed/rainxch/core/data/network/HttpClientFactory.kt (1)
19-19:java.io.IOExceptionincommonMainlimits multiplatform portability.If the project ever adds a non-JVM target (e.g., iOS, JS), this import will break compilation. Consider using a Kotlin multiplatform-compatible exception type or moving this to an intermediate
jvmMainsource set.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/data/src/commonMain/kotlin/zed/rainxch/core/data/network/HttpClientFactory.kt` at line 19, The import of java.io.IOException in HttpClientFactory.kt is JVM-specific and will break non-JVM targets; remove the java.io.IOException import and either replace uses with the multiplatform-friendly kotlin.io.IOException (or a generic Exception) or move the JVM-specific code (and the import) into a jvmMain implementation of HttpClientFactory; update any references in functions or error handling in HttpClientFactory to use the chosen exception type so commonMain stays platform-agnostic.core/data/src/commonMain/kotlin/zed/rainxch/core/data/network/ProxyManager.kt (1)
20-36: Consider basic input validation for host and port.
setHttpProxyandsetSocksProxyaccept arbitrary strings and ints with no validation. An empty host or an out-of-range port (e.g., negative or > 65535) will propagate silently and only fail at connection time with an unhelpful error.Example validation
fun setHttpProxy( host: String, port: Int, username: String? = null, password: String? = null ) { + require(host.isNotBlank()) { "Proxy host must not be blank" } + require(port in 1..65535) { "Proxy port must be in range 1..65535" } _proxyConfig.value = ProxyConfig.Http(host, port, username, password) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/data/src/commonMain/kotlin/zed/rainxch/core/data/network/ProxyManager.kt` around lines 20 - 36, The setters setHttpProxy and setSocksProxy currently accept any host and port; add basic input validation before assigning _proxyConfig: trim and require host is not blank (host.isNullOrBlank -> throw IllegalArgumentException with a clear message) and require port in the valid TCP range (1..65535 -> throw IllegalArgumentException if out of range); perform the same checks in both functions and only set _proxyConfig = ProxyConfig.Http(...) or ProxyConfig.Socks(...) after validation so invalid values are rejected early.
🤖 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/androidMain/kotlin/zed/rainxch/core/data/network/HttpClientFactory.android.kt`:
- Around line 54-68: The code currently calls
java.net.Authenticator.setDefault(...) when proxyConfig.username is set, which
installs a JVM-global authenticator and can leak across client/proxy changes;
update the client-creation flow to clear the global authenticator when the proxy
config no longer requires SOCKS auth by calling
java.net.Authenticator.setDefault(null) whenever proxyConfig.username is null or
when replacing an existing SOCKS proxy (ensure this reset happens before/after
creating the new OkHttp client), and add a short comment documenting the
JVM-global limitation so future maintainers see why we must clear the
authenticator.
In
`@core/data/src/commonMain/kotlin/zed/rainxch/core/data/network/GitHubClientProvider.kt`:
- Around line 30-51: The initial HttpClient is created as initialValue then
immediately closed by the upstream map collector; fix by creating the initial
client once (call createGitHubHttpClient and assign currentClient to it) and
then build the _client StateFlow from proxyConfigFlow but skip the first
emission so the upstream map doesn’t close the initial client—i.e., compute
initialClient = createGitHubHttpClient(...); set currentClient = initialClient;
then use proxyConfigFlow.drop(1).map { proxyConfig -> mutex.withLock {
currentClient?.close(); val newClient = createGitHubHttpClient(...,
proxyConfig); currentClient = newClient; newClient } }.stateIn(scope,
SharingStarted.Eagerly, initialClient); reference symbols: _client,
proxyConfigFlow, currentClient, createGitHubHttpClient, mutex.withLock, stateIn.
---
Duplicate comments:
In
`@core/data/src/androidMain/kotlin/zed/rainxch/core/data/network/HttpClientFactory.android.kt`:
- Around line 31-45: The proxyAuthenticator currently always returns a new
request with credentials and force-unwraps proxyConfig.username, which can cause
infinite retry loops and unsafe null usage; inside the proxyAuthenticator lambda
(symbol: proxyAuthenticator) safely read username/password from proxyConfig
(avoid proxyConfig.username!!), and short-circuit returning null when the
request already has a "Proxy-Authorization" header or the response has been
challenged before (e.g., inspect response.request.header("Proxy-Authorization")
or count prior responses) so you only add
Credentials.basic(proxyConfig.username, proxyConfig.password.orEmpty()) once and
return null on subsequent challenges to prevent infinite retries.
In
`@core/data/src/jvmMain/kotlin/zed/rainxch/core/data/network/HttpClientFactory.jvm.kt`:
- Around line 36-38: The JVM/CIO branch ignores proxy credentials: when handling
ProxyConfig.Http in HttpClientFactory.jvm.kt (the ProxyBuilder.http(...) block)
capture proxyConfig.username and proxyConfig.password and set up proxy
authentication using the client's proxyAuthenticator (or equivalent) so requests
to authenticated HTTP proxies include a Proxy-Authorization header; update the
ProxyBuilder.http branch to create a ProxyBuilder with the proxy URL and, if
proxyConfig.username/password are present, configure a proxy authenticator that
applies basic auth using those values (mirror the Android implementation's use
of proxyAuthenticator while referencing ProxyConfig.Http, proxyConfig.username,
proxyConfig.password, and ProxyBuilder.http).
- Around line 40-42: The CIO engine branch silently ignores ProxyConfig.Socks
(using ProxyBuilder.socks), so update HttpClientFactory to either (A) switch to
the OkHttp engine on JVM when ProxyConfig.Socks is provided—construct the
HttpClient with the OkHttp engine and configure its proxy using the SOCKS
settings from ProxyConfig.Socks—or (B) fail fast by throwing an
UnsupportedOperationException when encountering ProxyConfig.Socks in the JVM/CIO
path; locate the branch handling ProxyConfig.Socks (where ProxyBuilder.socks is
called) and implement one of these two fixes so SOCKS proxy configuration is
honored or clearly rejected.
In
`@core/domain/src/commonMain/kotlin/zed/rainxch/core/domain/model/ProxyConfig.kt`:
- Around line 8-20: Http and Socks data classes leak credentials via the
auto-generated toString(); override toString() in both classes (Http and Socks)
to return a string that includes host, port and username but redacts password
(e.g., show "<redacted>" or null) so plain-text passwords are not printed;
implement the custom toString() in each data class (overriding toString())
instead of changing other behaviors so equals/hashCode remain the data class
defaults.
---
Nitpick comments:
In
`@core/data/src/androidMain/kotlin/zed/rainxch/core/data/network/HttpClientFactory.android.kt`:
- Around line 12-73: In createPlatformHttpClient, validate proxyConfig.port
before constructing InetSocketAddress for ProxyConfig.Http and
ProxyConfig.Socks: ensure port is within 1..65535 (reject 0 and negative values)
and handle invalid ports by throwing a clear IllegalArgumentException (including
the bad value and host) or by skipping proxy setup/falling back to
Proxy.NO_PROXY; update the branches for ProxyConfig.Http and ProxyConfig.Socks
(where InetSocketAddress(...) is used) to perform this check and fail-fast with
a descriptive error to avoid silent misbehavior.
In
`@core/data/src/commonMain/kotlin/zed/rainxch/core/data/network/GitHubClientProvider.kt`:
- Around line 56-59: Wrap close() to be safe against concurrent calls by
guarding with a closed flag or the existing mutex: either (A) add an
AtomicBoolean closed that you set true atomically at start of close() and return
if already closed, then close currentClient, set currentClient = null and cancel
scope; or (B) if you prefer to use the suspend mutex, acquire it from close()
via runBlocking { mutex.withLock { if (closed) return; closed = true;
currentClient?.close(); currentClient = null; scope.cancel() } } so you never
close a replaced client or race with proxy-change logic in mutex.withLock.
Ensure you reference and update currentClient, scope, mutex, and the new closed
flag consistently.
In
`@core/data/src/commonMain/kotlin/zed/rainxch/core/data/network/HttpClientFactory.kt`:
- Line 19: The import of java.io.IOException in HttpClientFactory.kt is
JVM-specific and will break non-JVM targets; remove the java.io.IOException
import and either replace uses with the multiplatform-friendly
kotlin.io.IOException (or a generic Exception) or move the JVM-specific code
(and the import) into a jvmMain implementation of HttpClientFactory; update any
references in functions or error handling in HttpClientFactory to use the chosen
exception type so commonMain stays platform-agnostic.
In
`@core/data/src/commonMain/kotlin/zed/rainxch/core/data/network/ProxyManager.kt`:
- Around line 20-36: The setters setHttpProxy and setSocksProxy currently accept
any host and port; add basic input validation before assigning _proxyConfig:
trim and require host is not blank (host.isNullOrBlank -> throw
IllegalArgumentException with a clear message) and require port in the valid TCP
range (1..65535 -> throw IllegalArgumentException if out of range); perform the
same checks in both functions and only set _proxyConfig = ProxyConfig.Http(...)
or ProxyConfig.Socks(...) after validation so invalid values are rejected early.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
core/data/src/androidMain/kotlin/zed/rainxch/core/data/network/HttpClientFactory.android.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/ProxyManager.ktcore/data/src/jvmMain/kotlin/zed/rainxch/core/data/network/HttpClientFactory.jvm.ktcore/domain/src/commonMain/kotlin/zed/rainxch/core/domain/model/ProxyConfig.kt
Comment on lines
+54
to
+68
| if (proxyConfig.username != null) { | ||
| java.net.Authenticator.setDefault(object : java.net.Authenticator() { | ||
| override fun getPasswordAuthentication(): PasswordAuthentication? { | ||
| if (requestingHost == proxyConfig.host && | ||
| requestingPort == proxyConfig.port | ||
| ) { | ||
| return PasswordAuthentication( | ||
| proxyConfig.username, | ||
| proxyConfig.password.orEmpty().toCharArray() | ||
| ) | ||
| } | ||
| return null | ||
| } | ||
| }) | ||
| } |
Contributor
There was a problem hiding this comment.
Authenticator.setDefault() sets process-global state — affects all Java networking and leaks across proxy changes.
java.net.Authenticator.setDefault() is JVM-wide. This means:
- It affects all socket connections in the process, not just this OkHttp client.
- When the proxy config changes (e.g., switching from SOCKS to None),
GitHubClientProvidercreates a new client but the old global authenticator remains installed. - Concurrent clients or other libraries making network calls will unexpectedly use this authenticator.
There isn't a clean per-client SOCKS auth solution in OkHttp, but at minimum the authenticator should be reset to null when the proxy config changes away from SOCKS. Consider documenting this limitation or exploring alternative approaches (e.g., SSH tunneling, a local SOCKS-to-HTTP bridge).
🤖 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/network/HttpClientFactory.android.kt`
around lines 54 - 68, The code currently calls
java.net.Authenticator.setDefault(...) when proxyConfig.username is set, which
installs a JVM-global authenticator and can leak across client/proxy changes;
update the client-creation flow to clear the global authenticator when the proxy
config no longer requires SOCKS auth by calling
java.net.Authenticator.setDefault(null) whenever proxyConfig.username is null or
when replacing an existing SOCKS proxy (ensure this reset happens before/after
creating the new OkHttp client), and add a short comment documenting the
JVM-global limitation so future maintainers see why we must clear the
authenticator.
Comment on lines
+30
to
+51
| private val _client: StateFlow<HttpClient> = proxyConfigFlow | ||
| .map { proxyConfig -> | ||
| mutex.withLock { | ||
| currentClient?.close() | ||
| val newClient = createGitHubHttpClient( | ||
| tokenStore = tokenStore, | ||
| rateLimitRepository = rateLimitRepository, | ||
| proxyConfig = proxyConfig | ||
| ) | ||
| currentClient = newClient | ||
| newClient | ||
| } | ||
| } | ||
| .stateIn( | ||
| scope = scope, | ||
| started = SharingStarted.Eagerly, | ||
| initialValue = createGitHubHttpClient( | ||
| tokenStore = tokenStore, | ||
| rateLimitRepository = rateLimitRepository, | ||
| proxyConfig = proxyConfigFlow.value | ||
| ).also { currentClient = it } | ||
| ) |
Contributor
There was a problem hiding this comment.
Race condition: initial client is created then immediately closed and replaced.
stateIn(Eagerly, initialValue = ...) creates the initial HttpClient at line 46-50, but immediately starts collecting the upstream proxyConfigFlow.map. The first emission passes the same ProxyConfig through map, which calls currentClient?.close() (closing the initial client) before the new StateFlow value propagates. During this window, _client.value still returns the now-closed initial client.
Suggested restructuring
class GitHubClientProvider(
private val tokenStore: TokenStore,
private val rateLimitRepository: RateLimitRepository,
proxyConfigFlow: StateFlow<ProxyConfig>
) {
private val scope = CoroutineScope(SupervisorJob() + Dispatchers.Default)
- private var currentClient: HttpClient? = null
private val mutex = Mutex()
+ `@Volatile`
+ private var currentClient: HttpClient = createGitHubHttpClient(
+ tokenStore = tokenStore,
+ rateLimitRepository = rateLimitRepository,
+ proxyConfig = proxyConfigFlow.value
+ )
- private val _client: StateFlow<HttpClient> = proxyConfigFlow
- .map { proxyConfig ->
- mutex.withLock {
- currentClient?.close()
- val newClient = createGitHubHttpClient(
- tokenStore = tokenStore,
- rateLimitRepository = rateLimitRepository,
- proxyConfig = proxyConfig
- )
- currentClient = newClient
- newClient
- }
- }
- .stateIn(
- scope = scope,
- started = SharingStarted.Eagerly,
- initialValue = createGitHubHttpClient(
- tokenStore = tokenStore,
- rateLimitRepository = rateLimitRepository,
- proxyConfig = proxyConfigFlow.value
- ).also { currentClient = it }
- )
+ init {
+ proxyConfigFlow
+ .drop(1) // skip initial value — already used to create currentClient
+ .onEach { proxyConfig ->
+ mutex.withLock {
+ currentClient.close()
+ currentClient = createGitHubHttpClient(
+ tokenStore = tokenStore,
+ rateLimitRepository = rateLimitRepository,
+ proxyConfig = proxyConfig
+ )
+ }
+ }
+ .launchIn(scope)
+ }
/** Get the current HttpClient (always up to date with proxy settings) */
- val client: HttpClient get() = _client.value
+ val client: HttpClient get() = currentClient
fun close() {
- currentClient?.close()
+ currentClient.close()
scope.cancel()
}
}This avoids creating a throwaway client and eliminates the window where a closed client can be returned. Additional imports needed: drop, onEach, launchIn.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@core/data/src/commonMain/kotlin/zed/rainxch/core/data/network/GitHubClientProvider.kt`
around lines 30 - 51, The initial HttpClient is created as initialValue then
immediately closed by the upstream map collector; fix by creating the initial
client once (call createGitHubHttpClient and assign currentClient to it) and
then build the _client StateFlow from proxyConfigFlow but skip the first
emission so the upstream map doesn’t close the initial client—i.e., compute
initialClient = createGitHubHttpClient(...); set currentClient = initialClient;
then use proxyConfigFlow.drop(1).map { proxyConfig -> mutex.withLock {
currentClient?.close(); val newClient = createGitHubHttpClient(...,
proxyConfig); currentClient = newClient; newClient } }.stateIn(scope,
SharingStarted.Eagerly, initialClient); reference symbols: _client,
proxyConfigFlow, currentClient, createGitHubHttpClient, mutex.withLock, stateIn.
5 tasks
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 by CodeRabbit
New Features
Documentation