ADFA-4942: Gate GlitchTip and Firebase analytics behind an onboarding opt-out consent - #1617
ADFA-4942: Gate GlitchTip and Firebase analytics behind an onboarding opt-out consent#1617Daniel-ADFA wants to merge 2 commits into
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 Walkthrough
WalkthroughThe change replaces legacy privacy-disclosure tracking with persisted telemetry consent. Startup and analytics now require granted consent. Onboarding provides accept, decline, and learn-more actions. Tests cover persistence, migration, analytics, and StrictMode behavior. ChangesTelemetry consent
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
actor User
participant PermissionsFragment
participant StatPreferences
participant DeviceProtectedApplicationLoader
participant AnalyticsManager
User->>PermissionsFragment: select privacy slide
PermissionsFragment->>StatPreferences: read telemetryConsent
PermissionsFragment-->>User: show consent dialog
User->>PermissionsFragment: accept or decline
PermissionsFragment->>StatPreferences: persist GRANTED or DECLINED
PermissionsFragment->>DeviceProtectedApplicationLoader: initialize after acceptance
DeviceProtectedApplicationLoader->>AnalyticsManager: initialize telemetry
AnalyticsManager->>StatPreferences: verify granted consent
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 (4)
preferences/src/main/java/com/itsaky/androidide/preferences/internal/StatPreferences.kt (2)
35-49: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueCache fields are not synchronized across threads.
cachedPrefsandcachedPrefsAppare plainvarfields read and written from multiple threads:PermissionsFragmentwritestelemetryConsenton the main thread, whileDeviceProtectedApplicationLoader.initTelemetryIfConsentedreads it from aDispatchers.Defaultcoroutine. Without@Volatileor synchronization, one thread may not see another thread's write to these fields.In practice, the impact is limited because Android's
ContextImplalready cachesSharedPreferencesinstances per file path internally, so a stale read here just means an extra call togetSharedPreferences(), not incorrect data. Consider marking these fields@Volatilefor correctness under the Java Memory Model.🤖 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 `@preferences/src/main/java/com/itsaky/androidide/preferences/internal/StatPreferences.kt` around lines 35 - 49, Mark both cachedPrefs and cachedPrefsApp in the StatPreferences cache with `@Volatile` so updates are visible across threads under the Java Memory Model, while preserving the existing cache lookup and initialization behavior.
30-59: 📐 Maintainability & Code Quality | 🔵 TrivialAdd KDoc for the public consent API.
StatPreferences.telemetryConsenthas a non-obvious contract: it falls back toTelemetryConsent.UNSETfor missing or corrupt stored values, and writes are asynchronous throughapply(). Document this contract, including the threading expectations, since callers inDeviceProtectedApplicationLoaderandPermissionsFragmentread and write this property from different dispatchers.As per coding guidelines, "Public classes, functions, and non-obvious logic must have KDoc or Javadoc documenting contracts, rationale, threading, nullability, side effects, or units."
🤖 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 `@preferences/src/main/java/com/itsaky/androidide/preferences/internal/StatPreferences.kt` around lines 30 - 59, Add KDoc to the public StatPreferences.telemetryConsent property documenting that missing or invalid persisted values return TelemetryConsent.UNSET, writes use asynchronous SharedPreferences.apply(), and callers must follow the existing threading expectations when accessing it from different dispatchers.Source: Coding guidelines
app/src/main/java/com/itsaky/androidide/app/DeviceProtectedApplicationLoader.kt (1)
108-171: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd KDoc to the public telemetry-gating functions.
initTelemetryIfConsentedandonTelemetryConsentGrantedare public functions with non-obvious contracts: consent gating, a one-time atomic guard, and thread dispatch (Dispatchers.DefaultthenDispatchers.Main). Document these contracts for future maintainers.As per coding guidelines, "Public classes, functions, and non-obvious logic must have KDoc or Javadoc documenting contracts, rationale, threading, nullability, side effects, or units."
🤖 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/itsaky/androidide/app/DeviceProtectedApplicationLoader.kt` around lines 108 - 171, Add KDoc to the public functions initTelemetryIfConsented and onTelemetryConsentGranted documenting consent gating, the one-time atomic initialization guard, telemetry initialization side effects, and their threading behavior: the caller launch uses Dispatchers.Default and analytics initialization switches to Dispatchers.Main.Source: Coding guidelines
app/src/androidTest/kotlin/com/itsaky/androidide/helper/HandlePrivacyDisclosureHelper.kt (1)
29-74: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider covering the decline ("Keep offline") path in this helper.
This helper verifies the dialog's appearance and the accept path, but it does not click "Keep offline" and confirm
TelemetryConsent.DECLINEDpersists end-to-end. Since the opt-out flow is a primary objective of this PR, exercising the decline branch here would close the last gap in UI-level coverage for consent.🤖 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/androidTest/kotlin/com/itsaky/androidide/helper/HandlePrivacyDisclosureHelper.kt` around lines 29 - 74, Extend handlePrivacyDisclosure to exercise the decline path using the existing declineText control: click “Keep offline,” wait for the UI to become idle, and verify with the same persistence retry pattern that StatPreferences.telemetryConsent becomes TelemetryConsent.DECLINED. Preserve the existing accept-path assertions and dialog verification.
🤖 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/androidTest/kotlin/com/itsaky/androidide/helper/HandlePrivacyDisclosureHelper.kt`:
- Around line 29-74: Extend handlePrivacyDisclosure to exercise the decline path
using the existing declineText control: click “Keep offline,” wait for the UI to
become idle, and verify with the same persistence retry pattern that
StatPreferences.telemetryConsent becomes TelemetryConsent.DECLINED. Preserve the
existing accept-path assertions and dialog verification.
In
`@app/src/main/java/com/itsaky/androidide/app/DeviceProtectedApplicationLoader.kt`:
- Around line 108-171: Add KDoc to the public functions initTelemetryIfConsented
and onTelemetryConsentGranted documenting consent gating, the one-time atomic
initialization guard, telemetry initialization side effects, and their threading
behavior: the caller launch uses Dispatchers.Default and analytics
initialization switches to Dispatchers.Main.
In
`@preferences/src/main/java/com/itsaky/androidide/preferences/internal/StatPreferences.kt`:
- Around line 35-49: Mark both cachedPrefs and cachedPrefsApp in the
StatPreferences cache with `@Volatile` so updates are visible across threads under
the Java Memory Model, while preserving the existing cache lookup and
initialization behavior.
- Around line 30-59: Add KDoc to the public StatPreferences.telemetryConsent
property documenting that missing or invalid persisted values return
TelemetryConsent.UNSET, writes use asynchronous SharedPreferences.apply(), and
callers must follow the existing threading expectations when accessing it from
different dispatchers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 96611758-b0d8-47f1-ad0f-dd6800ce0a02
📒 Files selected for processing (12)
app/src/androidTest/kotlin/com/itsaky/androidide/app/strictmode/WhitelistRulesTest.ktapp/src/androidTest/kotlin/com/itsaky/androidide/helper/HandlePrivacyDisclosureHelper.ktapp/src/main/AndroidManifest.xmlapp/src/main/java/com/itsaky/androidide/analytics/AnalyticsManager.ktapp/src/main/java/com/itsaky/androidide/app/DeviceProtectedApplicationLoader.ktapp/src/main/java/com/itsaky/androidide/app/strictmode/WhitelistEngine.ktapp/src/main/java/com/itsaky/androidide/fragments/onboarding/PermissionsFragment.ktapp/src/test/java/com/itsaky/androidide/analytics/AnalyticsManagerConsentTest.ktapp/src/test/java/com/itsaky/androidide/app/TelemetryConsentMigrationTest.ktapp/src/test/java/com/itsaky/androidide/preferences/StatPreferencesTest.ktpreferences/src/main/java/com/itsaky/androidide/preferences/internal/StatPreferences.ktresources/src/main/res/values/strings.xml
| @Test | ||
| fun allow_DiskRead_on_TelemetryConsentPrefsResolution() { | ||
| assertAllowed<DiskReadViolation>( | ||
| // @formatter:off | ||
| stackTraceElement("java.io.File", "exists"), | ||
| stackTraceElement("android.app.ContextImpl", "getDataDir"), | ||
| stackTraceElement("android.app.ContextImpl", "getPreferencesDir"), | ||
| stackTraceElement("android.app.ContextImpl", "getSharedPreferencesPath"), | ||
| stackTraceElement("android.app.ContextImpl", "getSharedPreferences"), | ||
| stackTraceElement("com.itsaky.androidide.preferences.internal.StatPreferences", "getPrefs"), | ||
| stackTraceElement("com.itsaky.androidide.preferences.internal.StatPreferences", "getTelemetryConsent"), | ||
| // @formatter:on | ||
| ) | ||
| } | ||
|
|
||
| @Test | ||
| fun allow_DiskRead_on_TelemetryConsentFirstRead() { | ||
| assertAllowed<DiskReadViolation>( | ||
| // @formatter:off | ||
| stackTraceElement("android.os.StrictMode\$AndroidBlockGuardPolicy", "onReadFromDisk"), | ||
| stackTraceElement("android.app.SharedPreferencesImpl", "awaitLoadedLocked"), | ||
| stackTraceElement("android.app.SharedPreferencesImpl", "getString"), | ||
| stackTraceElement("com.itsaky.androidide.preferences.internal.StatPreferences", "getTelemetryConsent"), | ||
| // @formatter:on | ||
| ) | ||
| } |
There was a problem hiding this comment.
These seem to be originated from first-party code. Please look into fixing the violations instead of whitelisting them.
| rule { | ||
| ofType<DiskReadViolation>() | ||
| allow( | ||
| """ | ||
| StatPreferences stores the telemetry consent in device-protected | ||
| SharedPreferences. Resolving and loading that file is a once-per-process | ||
| read on the startup path that gates telemetry init (ADFA-4942), and cannot | ||
| be deferred. | ||
| """.trimIndent(), | ||
| ) | ||
|
|
||
| matchFramesInOrder( | ||
| anyOf( | ||
| classAndMethod("android.app.ContextImpl", "getSharedPreferences"), | ||
| classAndMethod("android.app.SharedPreferencesImpl", "awaitLoadedLocked"), | ||
| ), | ||
| classEquals("com.itsaky.androidide.preferences.internal.StatPreferences"), | ||
| ) | ||
| } |
There was a problem hiding this comment.
This seems to be originated from first-party code. Please look into fixing the violations instead of whitelisting them.
If it is absolutely not possible to use deferred logic, in that case you can use the allowThreadDiskReads for that piece of code. Example:
import com.itsaky.androidide.utils.allowThreadDiskReads
val diskReadResult = allowThreadDiskReads("reason") {
// read disk here
}| .also { dialog -> | ||
| dialog.getButton(AlertDialog.BUTTON_NEUTRAL).setOnClickListener { | ||
| openPrivacyPolicy() |
There was a problem hiding this comment.
We're passing null to setNeutralButton's second parameter above, why not use that instead of adding an OnClickListener here manually?
3 participants
Adds an opt-out for GlitchTip and Firebase analytics (ADFA-4942): one-time consent at onboarding, no Settings toggle, accept as the prominent default (per ticket design). Users exposed to network-level surveillance (SNI/DPI) can keep CoGo fully offline.
How it works
UNSET/GRANTED/DECLINED) in device-protected SharedPreferences (StatPreferences).DeviceProtectedApplicationLoaderinits telemetry only onGRANTED; Accept triggers the same idempotent path on first run.AnalyticsManager's collection flag is consent-driven, so ungated track calls can't re-enable it for declined users.GRANTEDonce.Demo
Screen_recording_20260803_162156.webm