Conversation
tevincent
force-pushed
the
generate-crypto-objects
branch
from
929413e to
d686fe8
Compare
tevincent
force-pushed
the
improve-network-code
branch
from
7fa7db5 to
538b974
Compare
tevincent
force-pushed
the
generate-crypto-objects
branch
from
d96a47e to
56913a4
Compare
|
This PR/issue depends on:
|
tevincent
force-pushed
the
improve-network-code
branch
from
49f6d15 to
d85419f
Compare
|
The refactoring improves the networking architecture by centralizing environment configuration in ApiClientProvider using Ktor's defaultRequest plugin, eliminating the redundant BaseRequest abstraction and simplifying repository constructors. The migration to direct HttpClient usage with extension functions provides cleaner request syntax, though the deletePasskey method inconsistently uses a hardcoded URL path instead of ApiRoutes constants like other endpoints. #ai-review-summary |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // When you don't use AuthenticatorInjection, you don't have an userAgent, so we're currently setting a default value. | ||
| // See later how to improve it. | ||
| private val userAgent: String = "Ktor client", | ||
| class ApiClientProvider constructor( |
There was a problem hiding this comment.
Class visibility changed from 'internal' to 'public' - confirm this exposure to public API is intentional.
| class ApiClientProvider constructor( | |
| internal class ApiClientProvider constructor( |
#ai-review-inline
| coerceInputValues = true // Use default values if not recognized (used for enums). | ||
| ignoreUnknownKeys = true // Don't break if keys are added. | ||
| @OptIn(ExperimentalSerializationApi::class) | ||
| decodeEnumsCaseInsensitive = true |
There was a problem hiding this comment.
Missing 'useAlternativeNames = false' setting from previous JSON configuration, which may change serialization behavior for classes with @SerialName annotations.
| decodeEnumsCaseInsensitive = true | |
| decodeEnumsCaseInsensitive = true | |
| useAlternativeNames = false |
#ai-review-inline
| crashReport = crashReport, | ||
| ) | ||
|
|
||
| val json = Json { |
There was a problem hiding this comment.
Class-level 'json' property is no longer used by the HTTP client configuration, creating duplicate configuration and maintenance confusion.
#ai-review-inline
| // When you don't use AuthenticatorInjection, you don't have an userAgent, so we're currently setting a default value. | ||
| // See later how to improve it. | ||
| private val userAgent: String = "Ktor client", | ||
| class ApiClientProvider constructor( |
There was a problem hiding this comment.
Removal of HttpClientEngine parameter reduces testability by preventing injection of mock/test engines.
#ai-review-inline
|
|
||
| suspend fun deletePasskey(token: String, passkeyId: String) { | ||
| return delete(createUrl("users/me/passkeys/$passkeyId"), appendHeaders = { | ||
| httpClient.delete(Url("users/me/passkeys/$passkeyId")) { |
There was a problem hiding this comment.
Inconsistent response handling: 'deletePasskey' does not decode the response or handle errors like other methods (get/challenge/verify).
| httpClient.delete(Url("users/me/passkeys/$passkeyId")) { | |
| httpClient.delete(Url("users/me/passkeys/$passkeyId")) { | |
| addAuthenticationHeader(token) | |
| }.decode<Unit>() |
#ai-review-inline
1 participant
depends on #41