Add ID confirmation dialog UI and improve project configuration - #254
Conversation
|
Warning Review limit reached
More reviews will be available in 39 minutes and 12 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughMainActivity now routes startup and runtime deep-links through a unified handler that parses study parameters and, when present, shows a confirmation dialog to set a pseudo user ID and optional consent; UI state prevents conflicting popups while confirmation is active. New dialog layout, drawables, strings, and an App Links intent-filter were added. ChangesID Confirmation Deep-Link & Dialog Workflow
Sequence DiagramsequenceDiagram
participant MainActivity
participant handleIncomingIntent
participant showConfirmIdDialog
participant SharedPreferences
participant AnalyticsUtils
participant AppLoader
MainActivity->>handleIncomingIntent: onCreate(getIntent()) / onNewIntent(intent)
handleIncomingIntent->>handleIncomingIntent: parse study_user_id, study_consent, confirmation_message, language
handleIncomingIntent->>showConfirmIdDialog: present confirmation UI when consent true & id provided
showConfirmIdDialog->>SharedPreferences: save pseudoId (and studyConsent)
showConfirmIdDialog->>AnalyticsUtils: log cr_user_id_confirmed
showConfirmIdDialog->>AppLoader: loadApps(selectedLanguage) or call showLanguagePopup
showConfirmIdDialog->>MainActivity: clear isHandlingIdConfirmation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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: 4
🤖 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 `@app/src/main/java/org/curiouslearning/container/MainActivity.java`:
- Line 324: The log statement in MainActivity's handleIncomingIntent currently
prints the full deep-link URI (variable data) which may contain sensitive query
params like study_user_id and consent values; change the logging to avoid
exposing query/content by either logging only non-sensitive components (e.g.,
data.getScheme(), data.getHost(), data.getPath()) or by redacting/stripping the
query and fragment before logging (construct a sanitized Uri without query
parameters) and then use that sanitized value in the Log.d(TAG, ...) call
instead of the full data.toString().
- Around line 340-347: The isHandlingIdConfirmation flag is only cleared in the
confirm button listener causing it to stay true on dialog
dismiss/cancel/exception; update showConfirmIdDialog (or wherever the
AlertDialog/Dialog instance is built) to reset isHandlingIdConfirmation in an
OnDismissListener and an OnCancelListener attached to the dialog, and wrap
dialog creation/display in a try/catch that clears isHandlingIdConfirmation in
the catch block as well so every exit path (confirm, dismiss, cancel, exception)
resets the flag.
- Around line 355-358: The code reads language via
data.getQueryParameter("language") and immediately calls charAt(0)/substring(1)
to set selectedLanguage, which will throw if language is an empty string; change
the logic in the block that sets selectedLanguage to first check that language
is not null and not empty (e.g., language.length() > 0 or !language.isEmpty())
before accessing charAt(0)/substring(1), and if empty treat it as null or use a
safe default; apply the identical guard-and-fallback fix to the other identical
block that constructs selectedLanguage (the copy at the second occurrence).
In `@app/src/main/res/layout/dialog_confirm_id.xml`:
- Line 19: The dialog uses hardcoded strings and symbols which break
localization and accessibility; move the literal message, the placeholder ID
text, and the checkmark ("✔") into string resources (strings.xml) and reference
them via android:text="`@string/`..." in dialog_confirm_id.xml, and replace the
visual checkmark with a semantic confirm label (e.g., `@string/confirm`) or add
android:contentDescription referencing a string resource so screen readers get
meaningful text; update any TextView/Button attributes in dialog_confirm_id.xml
that currently contain the hardcoded text to use these new string resource
identifiers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f0a61cec-b195-4473-bf08-d840914b816c
📒 Files selected for processing (5)
app/src/main/java/org/curiouslearning/container/MainActivity.javaapp/src/main/res/drawable/bg_button_confirm.xmlapp/src/main/res/drawable/bg_dialog.xmlapp/src/main/res/drawable/bg_id_field.xmlapp/src/main/res/layout/dialog_confirm_id.xml
dz4va
left a comment
dz4va
left a comment
There was a problem hiding this comment.
Let's also apply the Coderabbit suggestion fixes @amitsinghsutara
Hey @dz4va , |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/main/java/org/curiouslearning/container/MainActivity.java (1)
355-365:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid persisting or logging empty
languagevalues.At Line 355 and Line 619 paths, an empty query value (
?language=) is still treated as a value: it clears stored language, can triggerloadApps(""), and in the deferred flow can log alanguage_selectedevent with an empty language. Treat empty/blank as invalid and exit those branches without persisting or logging language selection.Suggested fix
// handleIncomingIntent(...) String language = data.getQueryParameter("language"); -if (language != null) { - if (language.length() > 0) { +if (language != null && !language.trim().isEmpty()) { + if (language.length() > 0) { selectedLanguage = Character.toUpperCase(language.charAt(0)) + language.substring(1).toLowerCase(); - } else { - selectedLanguage = ""; } storeSelectLanguage(selectedLanguage); runOnUiThread(() -> { loadApps(selectedLanguage); }); +} else if (language != null) { + Log.w(TAG, "handleIncomingIntent: empty language ignored."); }// fetchFacebookDeferredData(...) validLanguage(language, "facebook", String.valueOf(deepLinkUri)); -String lang = ""; -if (language != null && language.length() > 0) { +if (language == null || language.trim().isEmpty()) { + return; // validLanguage() already handles invalid language flow +} +String lang = ""; +if (language.length() > 0) { lang = Character.toUpperCase(language.charAt(0)) + language.substring(1).toLowerCase(); }Also applies to: 618-631
🤖 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/org/curiouslearning/container/MainActivity.java` around lines 355 - 365, The code currently treats empty or blank language strings as valid and persists/acts on them; modify the branches around selectedLanguage handling (the block that calls storeSelectLanguage(selectedLanguage) and runOnUiThread(() -> loadApps(selectedLanguage))) so you first trim the incoming language, check if it's null or empty after trimming, and if so return/skip the branch without calling storeSelectLanguage, loadApps, or emitting any language_selected events; apply the same guard in the deferred/logging flow that emits the language_selected event (ensure the event is only logged when the trimmed selectedLanguage is non-empty).
🤖 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 `@app/src/main/AndroidManifest.xml`:
- Around line 88-96: The intent-filter currently uses two separate <data>
elements which act as OR; replace them with a single <data> element that
includes both android:scheme="https" and
android:host="curiousreader.curiouscontent.org" so the filter only matches HTTPS
for that host (update the <intent-filter> block where the two <data> entries are
present and remove the standalone scheme-only and host-only <data> elements).
---
Outside diff comments:
In `@app/src/main/java/org/curiouslearning/container/MainActivity.java`:
- Around line 355-365: The code currently treats empty or blank language strings
as valid and persists/acts on them; modify the branches around selectedLanguage
handling (the block that calls storeSelectLanguage(selectedLanguage) and
runOnUiThread(() -> loadApps(selectedLanguage))) so you first trim the incoming
language, check if it's null or empty after trimming, and if so return/skip the
branch without calling storeSelectLanguage, loadApps, or emitting any
language_selected events; apply the same guard in the deferred/logging flow that
emits the language_selected event (ensure the event is only logged when the
trimmed selectedLanguage is non-empty).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a0650c22-00b3-4351-8f7d-fef4448e8c6e
📒 Files selected for processing (4)
app/src/main/AndroidManifest.xmlapp/src/main/java/org/curiouslearning/container/MainActivity.javaapp/src/main/res/layout/dialog_confirm_id.xmlapp/src/main/res/values/strings.xml
2 participants
Summary by CodeRabbit
New Features
UI
Bug Fixes