Conversation
…tions in build scripts
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughRefactors analytics and crash reporting into abstract interfaces, adds FOSS/Play product flavors with conditional dependencies and build changes, replaces Clarity/Firebase implementations with flavor-specific bindings (no-op for FOSS, Clarity+Firebase for Play), and updates CI/release workflows and UI masking usage. Changes
Sequence Diagram(s)sequenceDiagram
participant VM as ViewModel / UI
participant Tracker as AnalyticsTracker (interface)
participant Impl as MicrosoftClarityTracker (Play) / NoOp (FOSS)
participant Clarity as Microsoft Clarity SDK
participant Crash as CrashReporter -> FirebaseCrashlytics (Play) / NoOp (FOSS)
VM->>Tracker: trackEvent(name, value)
Tracker->>Impl: trackEvent(name, value)
alt Impl not initialized (consent not given)
Impl-->>Impl: enqueue event
else consent accepted / initialized
Impl->>Clarity: send event
end
VM->>Crash: recordException(Throwable)
Crash->>Crash: delegate to FirebaseCrashlytics or no-op
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 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: 3
🤖 Fix all issues with AI agents
In @.github/workflows/release.yml:
- Line 53: The release workflow currently hardcodes the release body as the
string "Update the Changelog" (the body field in the release step), which will
be used for every release; update the release job to either enable automatic
release notes by replacing the static body with the provider's dynamic option
(e.g., set generate_release_notes: true for the softprops/action-gh-release
step) or replace the literal with a meaningful placeholder or templated content
that incorporates changelog or commit info so releases are descriptive.
In
`@app/src/play/kotlin/org/grakovne/lissen/analytics/MicrosoftClarityTracker.kt`:
- Around line 32-41: The updateConsent method calls Clarity.pause() unguarded
which can fail if Clarity was never initialized; add an initialization guard:
introduce a private boolean (e.g., clarityInitialized) on the
MicrosoftClarityTracker class, set it to true immediately after calling
Clarity.initialize(...) in updateConsent (and false if you fully dispose), and
change the else branch to only call Clarity.pause() when clarityInitialized is
true (or alternatively check Clarity.isInitialized() if that API exists) so
pause is never invoked before initialization.
- Around line 17-26: trackEvent and setUser call Clarity.sendCustomEvent /
Clarity.setCustomUserId before Clarity.initialize() may finish; add an
initialization guard in MicrosoftClarityTracker so these methods never call
Clarity APIs until initialize completes: add an internal Boolean (e.g.,
clarityInitialized) set to true when updateConsent(true) completes its
Clarity.initialize() callback/promise, and in trackEvent(name, value) and
setUser(userId) check clarityInitialized first — if not initialized either queue
the event/userId to a small in-memory list to be flushed after initialization or
short-circuit and log/drop, but do not call Clarity.* methods; ensure
updateConsent(true) transitions clarityInitialized and flushes any queued items
after successful initialization.
🧹 Nitpick comments (4)
app/src/main/kotlin/org/grakovne/lissen/ui/screens/library/LibraryScreen.kt (1)
112-112: Consider usingcollectAsState()without an explicit initial value.If
isInitializingis aStateFlow, callingcollectAsState()(no argument) will use the flow's current value as initial state, staying in sync with the ViewModel. Passingtrueexplicitly means even if the ViewModel has already set it tofalsebefore composition, the UI will briefly seetrueand show the placeholder unnecessarily.Proposed fix
- val isInitializing by libraryViewModel.isInitializing.collectAsState(true) + val isInitializing by libraryViewModel.isInitializing.collectAsState()build.gradle.kts (1)
3-17: String concatenation obfuscation is redundant given the existing variant system.The codebase already uses Gradle product flavors (
fossandplay) with variant-specific dependency declarations (add("playImplementation", ...)), making the obfuscated strings unnecessary. The split strings ("com." + "google","fire" + "base") appear to be kept primarily for F-Droid scanner compatibility (as noted in the code comment at line 152 of app/build.gradle.kts), but they add no functional benefit and hurt readability.If F-Droid scanner compatibility requires the
FOSS_REMOVE_START/ENDmarkers, document this requirement clearly in the README or build documentation. Otherwise, remove the obfuscation and rely solely on the variant system, which is cleaner and more maintainable:if (!isFossBuild) { apply(plugin = "com.google.gms.google-services") apply(plugin = "com.google.firebase.crashlytics") }app/build.gradle.kts (2)
86-98: Hardcoded Clarity project ID in build config.
CLARITY_PROJECT_IDis embedded directly in the build file (Line 97). While Clarity project IDs are generally considered public (they end up in client-side code), for consistency with how other credentials are handled in this file (env vars /local.propertiesfallback), consider externalizing it.Proposed change
create("play") { dimension = "distribution" buildConfigField("String", "APP_NAME_SUFFIX", "\"\"") buildConfigField("String", "DISTRIBUTION", "\"play\"") - buildConfigField("String", "CLARITY_PROJECT_ID", "\"vc8bgk8nk9\"") + buildConfigField( + "String", + "CLARITY_PROJECT_ID", + "\"${System.getenv("CLARITY_PROJECT_ID") ?: localProperties.getProperty("CLARITY_PROJECT_ID") ?: ""}\"" + ) }
152-167: Plugin obfuscation via string concatenation is fragile.The split-string approach (
"com." + "google","fire" + "base") to evade the F-Droid scanner is a known pattern but inherently brittle — any scanner update could defeat it. You already haveFOSS_REMOVE_START/FOSS_REMOVE_ENDmarkers, which is the more robust approach since F-Droid's build system can strip those sections entirely.Also,
isFossBuildat Lines 153-154 is evaluated at configuration time. If a Gradle invocation includes both Foss and Play tasks (e.g.,./gradlew assembleFossDebug assemblePlayRelease), the Play build would fail because the plugins won't be applied. This is likely acceptable for your use case (separate CI jobs per flavor), but worth documenting.
app/src/play/kotlin/org/grakovne/lissen/analytics/MicrosoftClarityTracker.kt
Show resolved
Hide resolved
app/src/play/kotlin/org/grakovne/lissen/analytics/MicrosoftClarityTracker.kt
Show resolved
Hide resolved
…lization and enable automatic release note generation in the release workflow.
1 participant
Summary by CodeRabbit
New Features
User-facing Changes
Documentation
Chores