Feat/foss things - #9
Conversation
…Play build variants.
…nd refresh associated graphics.
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughThe PR introduces flavor-based build variants (FOSS and Play) with flavor-specific analytics and crash reporting implementations. It abstracts crash reporting behind a Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(200,200,255,0.5)
participant App
participant DI
participant UI
end
rect rgba(200,255,200,0.5)
participant CrashReporter
participant Firebase
participant Clarity
end
App->>DI: inject CrashReporter (at startup)
DI-->>App: CrashReporter impl (Play: FirebaseCrashReporter / FOSS: NoOpCrashReporter)
UI->>App: user action / uncaught exception
App->>CrashReporter: recordException(ex) / setCollectionEnabled(flag)
alt Play flavor
CrashReporter->>Firebase: recordException / setCrashlyticsCollectionEnabled
App->>Clarity: clarity tracking via ClarityComponent (bound into RunningComponent)
else FOSS flavor
CrashReporter-->>App: no-op logging (local only)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 body:
"Update the Changelog" which is a placeholder; replace this with the automated
changelog output used previously (e.g., reference the output from the changelog
generation step like body: ${{ steps.generate_changelog.outputs.changelog }}) or
add a pre-release check that fails or prompts if the changelog is empty; locate
the hardcoded body string in the release.yml and wire it to the changelog
generation step (or replace it with a clear reminder/validation step) so actual
release notes are included automatically.
In
`@app/src/main/kotlin/org/grakovne/lissen/persistence/preferences/LissenSharedPreferences.kt`:
- Around line 639-654: The decrypt function currently only catches exceptions
from cipher.doFinal, so Base64.decode, iv/cipherText slicing,
Cipher.getInstance, GCMParameterSpec, cipher.init or getSecretKey() failures can
crash callers (getToken/getAccessToken/getRefreshToken); wrap the entire body of
decrypt(...) in a try-catch that records the exception via
crashReporter.recordException(ex) and returns null on any failure, ensuring all
pre-doFinal errors are handled and callers safely receive null.
In `@metadata/en-US/full_description.txt`:
- Line 17: Remove the trailing whitespace at the end of the sentence "Kahani is
focused on simplicity, privacy, and the joy of a good story." in the
full_description entry so the line ends exactly after the period (no space or
invisible characters), which will eliminate lint/diff noise.
🧹 Nitpick comments (7)
.github/workflows/build_app.yml (1)
20-20: Consider upgrading action versions to matchrelease.yml.
release.ymlusesactions/checkout@v4andactions/setup-java@v4, while this workflow still uses@v3for both. Aligning versions avoids subtle behavioral differences between workflows.app/src/main/kotlin/org/grakovne/lissen/ui/screens/settings/advanced/AdvancedSettingsComposable.kt (1)
114-120: Consider extracting the"play"magic string to a constant.The string
"play"is used here and inLibraryScreen.kt(Line 504) for distribution checks. A shared constant (e.g.,const val PLAY_DISTRIBUTION = "play"in a common location) would reduce the risk of typos and make distribution checks easier to find and maintain.app/src/main/kotlin/org/grakovne/lissen/ui/screens/library/LibraryScreen.kt (1)
112-112: ReplacecollectAsState(true)withcollectAsState()for simpler state management.Since
libraryViewModel.isInitializingis aStateFlow<Boolean>already initialized totruein the ViewModel, callingcollectAsState()without the parameter will automatically use the StateFlow's current value. This is cleaner and more maintainable, as it eliminates redundancy and ensures the composable automatically adapts if the ViewModel's initialization logic changes in the future.app/src/foss/kotlin/org/grakovne/lissen/common/CrashHandler.kt (1)
35-36: Consider logging the swallowed exception for debuggability.The
catchblock silently discards exceptions occurring within the crash handler itself. While ignoring them is reasonable to avoid recursive crashes, logging withTimber.e(orLog.eto avoid Timber dependency issues in a crash path) would aid debugging if the toast display ever fails silently.Proposed fix
} catch (e: Exception) { - // Ignore errors in the crash handler itself + // Best-effort log; avoid rethrowing to prevent recursive crashes + android.util.Log.e("CrashHandler", "Error in crash handler", e) } finally {app/src/main/kotlin/org/grakovne/lissen/viewmodel/LibraryViewModel.kt (1)
64-65:_isInitializingcan be prematurely cleared when concurrentrefreshLibrarycalls overlap.
refreshLibraryis called fromcheckRefreshNeeded, theinitserver-availability collector, and potentially from the UI (withforceRefresh). Each call launches a separate coroutine with its ownfinally { _isInitializing.value = false }. If two calls overlap, the first to finish will clear the flag while the second is still running.For a loading indicator this is likely tolerable, but worth noting if the initializing state is used to gate important UI behavior (e.g., preventing user interaction).
Also applies to: 238-273
app/build.gradle.kts (2)
99-99: Hardcoded Clarity project ID in the build file.
CLARITY_PROJECT_IDis embedded directly in the Gradle config. While not a secret (it's a client-side analytics identifier), consider managing it vialocal.propertiesor an environment variable for consistency with how signing config is handled, and to make it easier to change per-environment without modifying the build script.
145-152: Task-name-based filtering is fragile but pragmatic.Disabling Google Services / Crashlytics tasks by matching on task name strings (
contains("Foss"),contains("GoogleServices")) is a well-known workaround for flavor-aware plugin disabling. Just be aware this can break if AGP or the Google Services plugin renames tasks in a future version.
| tag_name: v${{ steps.get_version.outputs.version }} | ||
| name: Kahani v${{ steps.get_version.outputs.version }} | ||
| body: ${{ steps.build_changelog.outputs.changelog }} | ||
| body: "Update the Changelog" |
There was a problem hiding this comment.
Placeholder release body — intended?
"Update the Changelog" appears to be a TODO placeholder rather than actual release notes. The previous workflow apparently generated a changelog automatically. Consider restoring automated changelog generation or adding a reminder to update this before tagging a release.
🤖 Prompt for AI Agents
In @.github/workflows/release.yml at line 53, The release workflow currently
hardcodes the release body as body: "Update the Changelog" which is a
placeholder; replace this with the automated changelog output used previously
(e.g., reference the output from the changelog generation step like body: ${{
steps.generate_changelog.outputs.changelog }}) or add a pre-release check that
fails or prompts if the changelog is empty; locate the hardcoded body string in
the release.yml and wire it to the changelog generation step (or replace it with
a clear reminder/validation step) so actual release notes are included
automatically.
| • Home Widget: Control your current story directly from your home screen. | ||
| • Network Aware: Intelligently manages offline/online states and server availability. | ||
|
|
||
| Kahani is focused on simplicity, privacy, and the joy of a good story. |
There was a problem hiding this comment.
Remove trailing whitespace.
Minor cleanup to avoid diff noise and lint warnings in some pipelines.
🧹 Proposed fix
-Kahani is focused on simplicity, privacy, and the joy of a good story.
+Kahani is focused on simplicity, privacy, and the joy of a good story.📝 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.
| Kahani is focused on simplicity, privacy, and the joy of a good story. | |
| Kahani is focused on simplicity, privacy, and the joy of a good story. |
🤖 Prompt for AI Agents
In `@metadata/en-US/full_description.txt` at line 17, Remove the trailing
whitespace at the end of the sentence "Kahani is focused on simplicity, privacy,
and the joy of a good story." in the full_description entry so the line ends
exactly after the period (no space or invisible characters), which will
eliminate lint/diff noise.
… its `try-catch` scope.
1 participant
Summary by CodeRabbit
New Features
Chores