feat(mobile): align with the Mobile API contract (register/forgot UI, feature-flag gating)#5
Conversation
|
Warning Review limit reached
More reviews will be available in 13 minutes and 12 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?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 credits. 🚦 How do rate 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 see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
WalkthroughAggiornati i contratti API e i modelli condivisi per registrazione, cambio password, catalogo, prestiti, prenotazioni, notifiche e disponibilità libri. Nell’app sono stati aggiunti i flussi di registrazione e recupero password, nuove route, e schermate che reagiscono ai feature flag e alle nuove localizzazioni. ChangesAggiornamenti API e flussi auth mobile
Sequence Diagram(s)sequenceDiagram
participant LoginScreen
participant PinakesNavHost
participant RegisterScreen
participant ForgotPasswordScreen
participant AuthRepository
alt create account
LoginScreen->>PinakesNavHost: onRegister()
PinakesNavHost->>RegisterScreen: navigate Routes.REGISTER
RegisterScreen->>AuthRepository: auth.register(...)
else recover password
LoginScreen->>PinakesNavHost: onForgotPassword()
PinakesNavHost->>ForgotPasswordScreen: navigate Routes.FORGOT_PASSWORD
ForgotPasswordScreen->>AuthRepository: auth.forgotPassword(email)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
_contract/endpoint-manifest.spec.js (1)
301-301: 🗄️ Data Integrity & Integration | 🔵 TrivialVerifica che
wishlist.created_atabbia un default lato DB.La riga 301 utilizza
INSERT IGNOREe cattura silenziosamente eventuali errori:
try { dbExec(\INSERT IGNORE INTO wishlist (utente_id, libro_id) VALUES (...)`); } catch {}`Se la colonna
created_atnella tabellawishlistè definitaNOT NULLsenza un valore di default (es.DEFAULT CURRENT_TIMESTAMP), l'inserimento fallirà (poiché non viene fornito il valore) ma l'errore verrà ignorato.
Ciò porta il seed a diventare un no-op silenzioso: la riga non viene inserita, la tabellawishlistrimane vuota, e le prove successive che si aspettavano un eliminazione non avranno effetti.Aggiungi
DEFAULT CURRENT_TIMESTAMPalla definizione dicreated_atnel DB o modifica l'insert per includere esplicitamente il timestamp.🤖 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 `@_contract/endpoint-manifest.spec.js` at line 301, The wishlist seed insert is swallowing failures, so ensure the `wishlist` table’s `created_at` is always satisfied. Update the DB schema for `wishlist.created_at` to have a default such as `CURRENT_TIMESTAMP`, or change the insert in the seed logic around the `dbExec` call to explicitly provide the timestamp so `INSERT IGNORE` cannot turn a missing value into a silent no-op.
🤖 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 `@_contract/openapi.json`:
- Around line 235-236: The openapi contract for privacy_acceptance currently
allows any boolean, so update the schema in the privacy_acceptance property to
constrain it to true only using the existing OpenAPI schema shape (for example
const or enum) so generated clients and docs reflect the required consent.
Locate the property definition inside the contract schema and keep the rest of
the registration payload unchanged.
- Around line 1757-1831: The new protected availability endpoint currently
documents only 401/404/500, so update its response block in the openapi schema
to also include 403 and 429 alongside the existing error responses. Use the same
style as other protected routes in openapi.json and ensure the route’s responses
stay consistent with the rest of the spec so generated clients and tests can
handle authorization and rate-limit failures.
In `@app/src/main/java/com/pinakes/app/data/repository/ProfileRepository.kt`:
- Around line 34-36: The changePassword method in ProfileRepository is
incorrectly hardcoding password_confirm to the new password, which prevents
callers from passing the user-entered confirmation. Update the repository API
and its ChangePasswordRequest call site so changePassword accepts the
confirmation value explicitly and forwards it unchanged to api.changePassword,
keeping it aligned with ChangePasswordRequest and the OpenAPI contract.
In `@app/src/main/java/com/pinakes/app/ui/screens/login/ForgotPasswordScreen.kt`:
- Around line 72-76: The forgot-password failure handling in
ForgotPasswordScreen is leaking backend-specific messages via res.message, which
can expose account existence details. Update the ApiResult.Failure branch in the
state update to always use the generic forgot-password error resource/text
instead of surfacing res.message directly, and keep the flow semantics aligned
with the existing auth.forgotPassword() behavior by normalizing all errors in
this screen.
In `@app/src/main/java/com/pinakes/app/ui/screens/login/RegisterScreen.kt`:
- Around line 81-117: The submit flow in RegisterScreen.submit can fire multiple
overlapping auth.register calls because it does not guard against re-entry while
loading is true. Add an early return at the start of submit() that checks the
current state and exits when a registration request is already in progress, and
keep the loading flag set/unset around the existing validation and
viewModelScope.launch logic so duplicate taps cannot trigger multiple requests.
- Around line 246-252: The privacy checkbox row in RegisterScreen is missing an
accessible label and the text is not interactive, so update the
Row/Checkbox/Text composition to use a toggleable modifier on the whole row and
bind it to the same privacyAccepted state handler used by Checkbox and
vm::onPrivacyChange. Keep the existing Checkbox and
stringResource(R.string.register_privacy) in RegisterScreen, but make the text
part of the tappable/announced control so screen readers read one combined
element and tapping the label toggles the checkbox.
---
Nitpick comments:
In `@_contract/endpoint-manifest.spec.js`:
- Line 301: The wishlist seed insert is swallowing failures, so ensure the
`wishlist` table’s `created_at` is always satisfied. Update the DB schema for
`wishlist.created_at` to have a default such as `CURRENT_TIMESTAMP`, or change
the insert in the seed logic around the `dbExec` call to explicitly provide the
timestamp so `INSERT IGNORE` cannot turn a missing value into a silent no-op.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4248c1ef-03c6-46b2-836d-d4b812abbb2f
📒 Files selected for processing (23)
_contract/MOBILE_API_SPEC.md_contract/endpoint-manifest.spec.js_contract/health-sample.json_contract/openapi.jsonapp/src/main/java/com/pinakes/app/data/model/Models.ktapp/src/main/java/com/pinakes/app/data/repository/AuthRepository.ktapp/src/main/java/com/pinakes/app/data/repository/ProfileRepository.ktapp/src/main/java/com/pinakes/app/data/store/FeatureStore.ktapp/src/main/java/com/pinakes/app/ui/navigation/MainScaffold.ktapp/src/main/java/com/pinakes/app/ui/navigation/PinakesNavHost.ktapp/src/main/java/com/pinakes/app/ui/navigation/Routes.ktapp/src/main/java/com/pinakes/app/ui/screens/contact/ContactScreen.ktapp/src/main/java/com/pinakes/app/ui/screens/login/ForgotPasswordScreen.ktapp/src/main/java/com/pinakes/app/ui/screens/login/LoginScreen.ktapp/src/main/java/com/pinakes/app/ui/screens/login/LoginViewModel.ktapp/src/main/java/com/pinakes/app/ui/screens/login/RegisterScreen.ktapp/src/main/java/com/pinakes/app/ui/screens/notifications/NotificationsScreen.ktapp/src/main/java/com/pinakes/app/ui/screens/onboarding/OnboardingViewModel.ktapp/src/main/java/com/pinakes/app/ui/screens/profile/ProfileScreen.kti18n/de.jsoni18n/en.jsoni18n/fr.jsoni18n/it.json
| Row(modifier = form, verticalAlignment = Alignment.CenterVertically) { | ||
| Checkbox(checked = state.privacyAccepted, onCheckedChange = vm::onPrivacyChange) | ||
| Text( | ||
| text = stringResource(R.string.register_privacy), | ||
| style = MaterialTheme.typography.bodySmall, | ||
| color = MaterialTheme.colorScheme.onSurfaceVariant, | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
cat -n app/src/main/java/com/pinakes/app/ui/screens/login/RegisterScreen.kt | sed -n '240,260p'Repository: fabiodalez-dev/Pinakes-Android
Length of output: 1412
🏁 Script executed:
head -100 app/src/main/java/com/pinakes/app/ui/screens/login/RegisterScreen.kt | grep -A 5 "^import"Repository: fabiodalez-dev/Pinakes-Android
Length of output: 2375
🏁 Script executed:
grep -n "val form\|form =" app/src/main/java/com/pinakes/app/ui/screens/login/RegisterScreen.kt | head -5Repository: fabiodalez-dev/Pinakes-Android
Length of output: 233
🏁 Script executed:
cat -n app/src/main/java/com/pinakes/app/ui/screens/login/RegisterScreen.kt | sed -n '1,50p'Repository: fabiodalez-dev/Pinakes-Android
Length of output: 2709
Associa semanticamente il testo privacy alla checkbox
Attualmente la Checkbox a riga 247 manca di un'etichetta accessibile per gli screen reader, e il testo a riga 249 non è cliccabile. TalkBack/VoiceOver leggeranno la casella senza contesto e non consentiranno di attivare il controllo cliccando sul testo.
Implementare una soluzione basata su modifier.toggleable() per rendere l'intera riga interattiva e unire il contenuto per la lettura:
💡 Patch proposta
+import androidx.compose.foundation.selection.toggleable
+import androidx.compose.ui.semantics.Role
+import androidx.compose.ui.semantics.semantics
- Row(modifier = form, verticalAlignment = Alignment.CenterVertically) {
- Checkbox(checked = state.privacyAccepted, onCheckedChange = vm::onPrivacyChange)
+ Row(
+ modifier = form
+ .toggleable(
+ value = state.privacyAccepted,
+ role = Role.Checkbox,
+ onValueChange = vm::onPrivacyChange,
+ )
+ .semantics(mergeDescendants = true) {},
+ verticalAlignment = Alignment.CenterVertically,
+ ) {
+ Checkbox(checked = state.privacyAccepted, onCheckedChange = null)
Text(
text = stringResource(R.string.register_privacy),
style = MaterialTheme.typography.bodySmall,
color = MaterialTheme.colorScheme.onSurfaceVariant,
+ modifier = Modifier.padding(start = Spacing.sm),
)
}Verificare il comportamento con uno screen reader dopo l'implementazione.
📝 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.
| Row(modifier = form, verticalAlignment = Alignment.CenterVertically) { | |
| Checkbox(checked = state.privacyAccepted, onCheckedChange = vm::onPrivacyChange) | |
| Text( | |
| text = stringResource(R.string.register_privacy), | |
| style = MaterialTheme.typography.bodySmall, | |
| color = MaterialTheme.colorScheme.onSurfaceVariant, | |
| ) | |
| Row( | |
| modifier = form | |
| .toggleable( | |
| value = state.privacyAccepted, | |
| role = Role.Checkbox, | |
| onValueChange = vm::onPrivacyChange, | |
| ) | |
| .semantics(mergeDescendants = true) {}, | |
| verticalAlignment = Alignment.CenterVertically, | |
| ) { | |
| Checkbox(checked = state.privacyAccepted, onCheckedChange = null) | |
| Text( | |
| text = stringResource(R.string.register_privacy), | |
| style = MaterialTheme.typography.bodySmall, | |
| color = MaterialTheme.colorScheme.onSurfaceVariant, | |
| modifier = Modifier.padding(start = Spacing.sm), | |
| ) | |
| } |
🤖 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/login/RegisterScreen.kt` around
lines 246 - 252, The privacy checkbox row in RegisterScreen is missing an
accessible label and the text is not interactive, so update the
Row/Checkbox/Text composition to use a toggleable modifier on the whole row and
bind it to the same privacyAccepted state handler used by Checkbox and
vm::onPrivacyChange. Keep the existing Checkbox and
stringResource(R.string.register_privacy) in RegisterScreen, but make the text
part of the tappable/announced control so screen readers read one combined
element and tapping the label toggles the checkbox.
…label (review) From the contract-coherence review: - RegisterViewModel.submit() validated only min length (>=8). Mirror the backend AuthController rules client-side so the user gets a clear error instead of a server 422: 8-72 chars AND at least one uppercase, lowercase and digit. - The confirm-password field reused profile_confirm_new_password (from the change-password flow); add a dedicated register_confirm_password label. New i18n keys (register_error_password_length, register_error_password_weak, register_confirm_password) added to all four locales. Verified: compileDebugKotlin BUILD SUCCESSFUL.
- RegisterScreen: guard submit() against duplicate registration requests while a call is in flight or already completed (account creation is not idempotent) - ForgotPasswordScreen: never surface the backend's raw failure message — a specific "user not found" enabled account enumeration; always show a generic error to preserve the neutral "if that email is registered…" semantics - ProfileRepository: changePassword now forwards the real confirmation value instead of hardcoding password_confirm = new, so a client-side mismatch is no longer masked at the repository layer - openapi contract: constrain privacy_acceptance to const:true and document 403/429 on the book availability route
|
Addressed the 5 inline review findings (verified each against the code):
Build green ( |
1 participant
Companion to the backend's mobile-API coherence PR. Supersedes #3 and #4.
RegisterScreen,ForgotPasswordScreen) wired into login/nav, sending the full backend payload (telefono, indirizzo, password_confirm, privacy_acceptance) with matching validation.messages/notifications/registration_enabledflags.Verification:
./gradlew :app:compileDebugKotlin→ BUILD SUCCESSFUL.Summary by CodeRabbit
Nuove funzionalità
Miglioramenti
Correzioni