Report TLS/certificate failures distinctly instead of a generic network error#23
Conversation
…r connection" SSLException is a subtype of IOException, so a TLS/certificate handshake failure was being caught by the generic IOException branch and mapped to ErrorCodes.NETWORK -> "Couldn't reach that address. Check the URL and your connection." That sends the user chasing DNS/Wi-Fi when the real cause is the server's certificate (e.g. a chain that terminates in an expired root, which Android's system trust store rejects even though a browser with its own root store reaches the same host fine). - Add ErrorCodes.TLS and catch SSLException BEFORE IOException in apiCall/apiResponse (ApiResult) and in bookClubCall/bookClubCallUnit (BookClubResult). - Map the new code to a certificate-specific message in the onboarding and login view models, with strings for all four locales (en/it/de/fr). Now the onboarding "Connect your library" step tells the user it's a certificate/TLS problem and to check the server certificate — no app-side trust is relaxed, the bad cert is still rejected, it's just reported accurately.
|
Warning Review limit reached
Next review available in: 41 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughViene introdotto un nuovo codice errore ChangesGestione errori TLS
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant ViewModel
participant ApiResult
participant Retrofit
ViewModel->>ApiResult: apiCall / bookClubCall
ApiResult->>Retrofit: esegue richiesta HTTPS
Retrofit-->>ApiResult: SSLException
ApiResult-->>ViewModel: Failure(ErrorCodes.TLS, message, 0)
ViewModel->>ViewModel: applyError mostra login_error_tls / onboarding_error_tls
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/src/main/java/com/pinakes/app/ui/screens/onboarding/OnboardingViewModel.kt (1)
72-79: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsiderare l'uso di
ErrorCodes.TLSal posto del letterale"tls".
OnboardingViewModel.applyErrorutilizza stringhe letterali ("network","tls","not_found") mentreLoginViewModelutilizza le costanti diErrorCodes. Questa inconsistenza cross-file aumenta il rischio di typo silenziosi: una stringa mal scritta non genererebbe errori di compilazione. Si raccomanda di allineare questo file allo stesso pattern diLoginViewModelusando le costanti diErrorCodes.♻️ Refactor proposto per allineamento con LoginViewModel
private fun applyError(state: OnboardingUiState, failure: ApiResult.Failure): OnboardingUiState = when (failure.code) { - "network" -> state.copy(error = null, errorRes = R.string.onboarding_error_network) - "tls" -> state.copy(error = null, errorRes = R.string.onboarding_error_tls) - "not_found" -> state.copy(error = null, errorRes = R.string.onboarding_error_not_found) + ErrorCodes.NETWORK -> state.copy(error = null, errorRes = R.string.onboarding_error_network) + ErrorCodes.TLS -> state.copy(error = null, errorRes = R.string.onboarding_error_tls) + ErrorCodes.NOT_FOUND -> state.copy(error = null, errorRes = R.string.onboarding_error_not_found) else -> if (failure.message.isNotBlank()) state.copy(error = failure.message, errorRes = null) else state.copy(error = null, errorRes = R.string.onboarding_error_generic) }🤖 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 `@app/src/main/java/com/pinakes/app/ui/screens/onboarding/OnboardingViewModel.kt` around lines 72 - 79, OnboardingViewModel.applyError is still matching failure codes with string literals instead of the shared ErrorCodes constants, which risks silent typos and inconsistent handling. Update the when branch in applyError to use the same ErrorCodes symbols as LoginViewModel (for example the TLS case and the other code checks) so the mapping stays consistent and compiler-safe across files.
🤖 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
`@app/src/main/java/com/pinakes/app/ui/screens/onboarding/OnboardingViewModel.kt`:
- Around line 72-79: OnboardingViewModel.applyError is still matching failure
codes with string literals instead of the shared ErrorCodes constants, which
risks silent typos and inconsistent handling. Update the when branch in
applyError to use the same ErrorCodes symbols as LoginViewModel (for example the
TLS case and the other code checks) so the mapping stays consistent and
compiler-safe across files.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 89921645-4677-457b-85e3-9b06050405d3
📒 Files selected for processing (8)
app/src/main/java/com/pinakes/app/data/network/ApiResult.ktapp/src/main/java/com/pinakes/app/data/network/BookClubResult.ktapp/src/main/java/com/pinakes/app/ui/screens/login/LoginViewModel.ktapp/src/main/java/com/pinakes/app/ui/screens/onboarding/OnboardingViewModel.kti18n/de.jsoni18n/en.jsoni18n/fr.jsoni18n/it.json
…t string literals Align applyError() with LoginViewModel — use ErrorCodes.NETWORK/TLS/NOT_FOUND instead of "network"/"tls"/"not_found" so a typo is a compile error, not a silently unmatched branch.
1 participant
Motivated by issue #16: the app shows "Couldn't reach that address. Check the URL and your connection." while a browser on the same phone reaches the same instance over HTTPS and gets a valid
/api/v1response. The screenshots make it clear the server is fine — so the message sends the user chasing the wrong thing.Root cause
SSLException(and its subtypesSSLHandshakeException/SSLPeerUnverifiedException) extendsIOException, so a TLS/certificate handshake failure was caught by the genericIOExceptionbranch and mapped toErrorCodes.NETWORK→ the "check your connection" copy. A certificate problem (e.g. a chain that terminates in an expired root, which Android's system trust store rejects even though Chrome — using its own root store — reaches the same host fine) therefore looked like a connectivity problem.Change
ErrorCodes.TLS; catchSSLExceptionbeforeIOExceptioninapiCall/apiResponse(ApiResult.kt) and inbookClubCall/bookClubCallUnit(BookClubResult.kt).OnboardingViewModelandLoginViewModel, with strings for all four locales (en/it/de/fr) via thei18n/*.jsonsource of truth.No app-side trust is relaxed — the bad certificate is still rejected. It's only reported accurately, so onboarding now says it's a certificate/TLS issue and to check the server certificate, instead of blaming the network.
Validation
I could not build locally (no Android SDK in my environment), so the Android CI on this PR (
assembleDebug+assembleRelease+lintDebug+ unit tests) is the gate. Thei18nkeys were added to all four locales so theGenerateI18nResTaskcodegen producesR.string.onboarding_error_tls/R.string.login_error_tls.Summary by CodeRabbit
Bug Fixes
Documentation