ADFA-4942: Gate GlitchTip and Firebase analytics behind an onboarding opt-out consent - #1617
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.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 Walkthrough
WalkthroughThe PR replaces the legacy privacy flag with persisted telemetry consent states. Startup and Firebase Analytics now require granted consent. Onboarding provides accept, decline, and learn-more actions. Tests cover migration, persistence, analytics, and dialog controls. ChangesTelemetry consent
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant PermissionsFragment
participant StatPreferences
participant DeviceProtectedApplicationLoader
participant AnalyticsManager
participant Firebase
User->>PermissionsFragment: choose telemetry consent
PermissionsFragment->>StatPreferences: persist GRANTED or DECLINED
PermissionsFragment->>DeviceProtectedApplicationLoader: initialize after GRANTED
DeviceProtectedApplicationLoader->>AnalyticsManager: initialize telemetry
AnalyticsManager->>Firebase: enable analytics collection
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 (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
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)
preferences/src/main/java/com/itsaky/androidide/preferences/internal/StatPreferences.kt (1)
65-72: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument the telemetry-consent property contract.
Add KDoc for the device-protected storage behavior and the
UNSETfallback for missing or invalid values.As per coding guidelines, “Public classes, functions, and non-obvious logic must have KDoc.”
Proposed KDoc
+ /** + * Stores the user's telemetry consent in device-protected preferences. + * + * Missing or invalid values return [TelemetryConsent.UNSET]. + */ var telemetryConsent: TelemetryConsent🤖 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 65 - 72, Add KDoc to the public telemetryConsent property in StatPreferences, documenting that it uses device-protected preferences and returns TelemetryConsent.UNSET when the stored value is missing or invalid. Keep the existing getter and setter behavior unchanged.Source: Coding guidelines
🤖 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
`@preferences/src/main/java/com/itsaky/androidide/preferences/internal/StatPreferences.kt`:
- Around line 47-58: Update the StatPreferences cache-miss path around
TELEMETRY_CONSENT to avoid synchronously loading SharedPreferences on the main
thread and remove the allowThreadDiskReads exemption. Probe or load the consent
state from a background context, keeping telemetry disabled until that
asynchronous state is available; preserve the existing in-memory preference
behavior once loaded.
---
Outside diff comments:
In
`@preferences/src/main/java/com/itsaky/androidide/preferences/internal/StatPreferences.kt`:
- Around line 65-72: Add KDoc to the public telemetryConsent property in
StatPreferences, documenting that it uses device-protected preferences and
returns TelemetryConsent.UNSET when the stored value is missing or invalid. Keep
the existing getter and setter behavior unchanged.
🪄 Autofix
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 Plus
Run ID: 97abb0b0-0a58-4282-b041-18020aef1bd0
📒 Files selected for processing (1)
preferences/src/main/java/com/itsaky/androidide/preferences/internal/StatPreferences.kt
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/src/test/java/com/itsaky/androidide/analytics/AnalyticsManagerConsentTest.kt (2)
37-37: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd KDoc to the added public test functions.
Kotlin member functions are public by default. These test functions lack KDoc for their consent state and expected collection behavior. Add concise KDoc to each function as required by the repository guidelines.
Also applies to: 45-45, 53-53, 61-61
🤖 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/test/java/com/itsaky/androidide/analytics/AnalyticsManagerConsentTest.kt` at line 37, Add concise KDoc to each newly added public test function in AnalyticsManagerConsentTest, including the tests around “track call before initialize keeps collection disabled” and the functions at the referenced locations. Document the consent state being exercised and the expected analytics collection behavior, while leaving the test logic unchanged.Source: Coding guidelines
37-42: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftConfirm the test framework policy for this class.
The added tests inherit
@RunWith(RobolectricTestRunner::class), which indicates a JUnit 4 runner. The repository guideline requires JUnit Jupiter forsrc/testtests. Migrate this class to the approved Jupiter/Robolectric setup, or confirm an explicit legacy exception.Also applies to: 45-50, 53-58, 61-69
🤖 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/test/java/com/itsaky/androidide/analytics/AnalyticsManagerConsentTest.kt` around lines 37 - 42, Update AnalyticsManagerConsentTest to use the approved JUnit Jupiter + Robolectric setup instead of the JUnit 4 runner inherited from `@RunWith`(RobolectricTestRunner::class). Adjust the class-level test annotations and execution style accordingly while keeping the existing test methods and mocks intact, or otherwise document an explicit legacy exception if this class is intentionally excluded from the JUnit Jupiter policy.Source: Coding guidelines
🤖 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/test/java/com/itsaky/androidide/analytics/AnalyticsManagerConsentTest.kt`:
- Around line 45-50: Align the test with its stated target by either renaming
`metric call before initialize keeps collection disabled` to describe
`trackProjectOpened`, or change the invocation to `trackMetric` with a
representative metric; ensure the final test name and method under test match.
---
Nitpick comments:
In
`@app/src/test/java/com/itsaky/androidide/analytics/AnalyticsManagerConsentTest.kt`:
- Line 37: Add concise KDoc to each newly added public test function in
AnalyticsManagerConsentTest, including the tests around “track call before
initialize keeps collection disabled” and the functions at the referenced
locations. Document the consent state being exercised and the expected analytics
collection behavior, while leaving the test logic unchanged.
- Around line 37-42: Update AnalyticsManagerConsentTest to use the approved
JUnit Jupiter + Robolectric setup instead of the JUnit 4 runner inherited from
`@RunWith`(RobolectricTestRunner::class). Adjust the class-level test annotations
and execution style accordingly while keeping the existing test methods and
mocks intact, or otherwise document an explicit legacy exception if this class
is intentionally excluded from the JUnit Jupiter policy.
🪄 Autofix
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 Plus
Run ID: fe13a9de-9e28-43ae-b69d-fd09f7eae734
📒 Files selected for processing (4)
app/src/main/java/com/itsaky/androidide/analytics/AnalyticsManager.ktapp/src/main/java/com/itsaky/androidide/fragments/onboarding/PermissionsFragment.ktapp/src/test/java/com/itsaky/androidide/analytics/AnalyticsManagerConsentTest.ktpreferences/src/main/java/com/itsaky/androidide/preferences/internal/StatPreferences.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/java/com/itsaky/androidide/fragments/onboarding/PermissionsFragment.kt
itsaky-adfa
left a comment
itsaky-adfa
left a comment
There was a problem hiding this comment.
@Daniel-ADFA We still want to verify that there are no new StrictMode issues during QA.
4 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