E6: client-side product telemetry foundation#462
Closed
rainxchzed wants to merge 39 commits intomainfrom
Closed
Conversation
Defines a new product-metrics telemetry pipeline distinct from the existing TelemetryRepository (which feeds the backend's ranking-signal pipeline at /v1/events). ProductTelemetry targets POST /v1/telemetry/events with an opt-out, schema-allowlisted event stream — consent is three-state (NotYetAsked / Granted / Denied) so the first-launch sheet can distinguish 'never asked' from 'user said no.' TweaksRepository gains getProductTelemetryConsent / setProductTelemetryConsent backed by a new DataStore key, kept independent of getTelemetryEnabled so accepting the E6 sheet does not auto-enable the legacy pipeline.
…sent gating
Implements ProductTelemetry against the new POST /v1/telemetry/events backend route. Owns: an ephemeral session_id (16 random bytes, base64url, fresh per process), a 500-entry ring buffer with mutex-protected enqueue/drain, a 30-second timer + 20-event threshold flush trigger, exponential backoff from 5s to 5min on transient failure, and a 2-second shutdown drain via flush(). Consent is checked at fire(), at flush() entry, and again after the network round-trip — never lets a stale-consent batch leave the device. props are filtered to String/Number/Boolean only at serialization time so a non-primitive value (a repo object, an exception) is dropped before it can leak. BackendApiClient gains postProductTelemetryEvents that POSTs the new {events: [...]} batch shape; the existing postEvents path is untouched. SharedModule.kt registers ProductTelemetry as a Koin single.
ProductTelemetryEvents.ALL mirrors the backend's TelemetryAllowlist; ProductTelemetryProps.ALLOWED enumerates the categorical / bucketed prop keys callers may use. Wiring sites use these constants instead of inline strings so a typo at a fire() call is a compile error rather than a silently dropped event. PrivacyAuditTest will pin both sets when KMP test infra is added (punch list).
TweaksState gains productTelemetryConsent (three-state); TweaksAction gains OnProductTelemetryToggled mapping a Boolean to Granted/Denied. ViewModel observes the new TweaksRepository flow on init and writes Granted/Denied on toggle. The Compose UI binding (a Switch row in the Privacy section pointing at OnProductTelemetryToggled) is left to a follow-up commit; this commit unblocks both the UI and any caller that needs to read consent state.
First wired event in the E6 pipeline. fire() is a no-op when consent is not Granted, so this is safe to land before the consent-sheet UX ships. Android attaches platform=android plus the live versionName from PackageManager. Desktop attaches platform=macos|windows|linux derived from os.name (the impl already stamps appVersion from BuildKonfig at the event-level field; props is for additional context). Other 19 event sites tracked in the punch list.
Punch list for taking the client to GA: consent UI, first-launch sheet, 19 events, PrivacyAuditTest, shutdown flush. Resolves the overlap E1 flagged: import events MIGRATE from TelemetryRepository to ProductTelemetry (delete the legacy calls in the same commit, no double-emission). search_executed and the per-repo signal events COEXIST with legacy on purpose — different identity (hashed device_id vs ephemeral session_id), different jobs (ranking signals vs aggregate metrics).
Contributor
|
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:
WalkthroughAdds a product telemetry system: domain contracts, buffered implementation, DI wiring, backend POST, UI consent flows, app lifecycle/session/crash instrumentation, first‑paint and cold‑start metrics, repository/service instrumentation, privacy audit tests, and shutdown flushes for Android and Desktop. Changes
Sequence DiagramsequenceDiagram
participant App as App (Android/Desktop)
participant ColdStart as ColdStart
participant Telemetry as ProductTelemetryImpl
participant Tweaks as TweaksRepository
participant Backend as BackendApiClient
participant Server as Server / telemetry/events
App->>ColdStart: markStart()
App->>Telemetry: fire("APP_LAUNCHED", props)
Telemetry->>Tweaks: read consent
alt consent == Granted
Telemetry->>Telemetry: enqueue event (buffer)
else
Telemetry->>Telemetry: drop / no-op
end
loop periodic flush
Telemetry->>Tweaks: read consent
alt consent == Granted
Telemetry->>Backend: postProductTelemetryEvents(batch)
Backend->>Server: POST /v1/telemetry/events
Server-->>Backend: 2xx / 429 / error
Backend-->>Telemetry: result
else
Telemetry->>Telemetry: clear buffer
end
end
App->>Telemetry: flush() on shutdown/crash (bounded timeout)
Telemetry->>Backend: final post (if consent granted)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/UpdateCheckWorker.kt (1)
94-107:⚠️ Potential issue | 🟠 MajorDon’t convert coroutine cancellation into
OPERATION_FAILEDtelemetry.
CancellationExceptionis currently caught byException; on exhausted retries this can emit a false failure event. Also guard telemetry emission to keep failure handling robust.🐛 Suggested fix
+import kotlin.coroutines.cancellation.CancellationException @@ - } catch (e: Exception) { + } catch (e: CancellationException) { + throw e + } catch (e: Exception) { Logger.e { "UpdateCheckWorker: Update check failed: ${e.message}" } if (runAttemptCount < 3) { Result.retry() } else { - productTelemetry.fire( - name = ProductTelemetryEvents.OPERATION_FAILED, - props = - mapOf( - ProductTelemetryProps.OP to "update", - ProductTelemetryProps.ERROR_CODE to (e::class.simpleName ?: "unknown"), - ), - ) + runCatching { + productTelemetry.fire( + name = ProductTelemetryEvents.OPERATION_FAILED, + props = + mapOf( + ProductTelemetryProps.OP to "update", + ProductTelemetryProps.ERROR_CODE to (e::class.simpleName ?: "unknown"), + ), + ) + }.onFailure { telemetryErr -> + Logger.w { "UpdateCheckWorker: failed to emit OPERATION_FAILED telemetry: ${telemetryErr.message}" } + } Result.failure() } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/UpdateCheckWorker.kt` around lines 94 - 107, The catch block in UpdateCheckWorker is converting coroutine CancellationException into an OPERATION_FAILED telemetry event; update the handler so CancellationException is not treated as a regular failure: inside the catch for Exception in the UpdateCheckWorker (the block that logs the error, checks runAttemptCount, calls Result.retry()/Result.failure(), and fires productTelemetry.fire), detect if e is a CancellationException (or inherits from Throwable that indicates cancellation) and rethrow it immediately (or return Result.retry/propagate cancellation) so it is not logged as OPERATION_FAILED; only call productTelemetry.fire for non-cancellation exceptions when runAttemptCount >= 3 and then return Result.failure(). Ensure you reference the same symbols: UpdateCheckWorker, runAttemptCount, productTelemetry.fire, ProductTelemetryEvents.OPERATION_FAILED, and ProductTelemetryProps.composeApp/src/jvmMain/kotlin/zed/rainxch/githubstore/DesktopApp.kt (2)
39-45: 🛠️ Refactor suggestion | 🟠 MajorMove
CrashReporter.install()to the first line ofmain.
ColdStart.markStart()now runs before crash logging is installed, so failures there won’t be captured in the session/crash logs.As per coding guidelines, "Install CrashReporter as the first line of DesktopApp.main to tee System.out/err to rotating session.log and crash logs."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composeApp/src/jvmMain/kotlin/zed/rainxch/githubstore/DesktopApp.kt` around lines 39 - 45, Move the CrashReporter.install() call to be the very first statement inside the main function so stdout/stderr are tee'd before anything else; specifically, in DesktopApp.main swap the order of CrashReporter.install() and ColdStart.markStart() (call CrashReporter.install() first, then ColdStart.markStart()) so any failures during ColdStart.markStart() are captured by the crash/session logging.
83-97:⚠️ Potential issue | 🟠 MajorDon’t count deep-link handoff helpers as real launches.
APP_LAUNCHEDfires beforeDesktopDeepLink.tryForwardToRunningInstance(...)exits the transient process. Opening a link into an already-running app will therefore inflate launch metrics even though no new desktop session was actually started.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composeApp/src/jvmMain/kotlin/zed/rainxch/githubstore/DesktopApp.kt` around lines 83 - 97, The APP_LAUNCHED telemetry is sent before checking deep-link handoff, causing transient handoff processes to be counted as real launches; modify the logic so that ProductTelemetry.fire(...) for ProductTelemetryEvents.APP_LAUNCHED (the call currently passing props = mapOf(ProductTelemetryProps.PLATFORM to desktopPlatformSlug())) only runs if no deepLinkArg was forwarded by DesktopDeepLink.tryForwardToRunningInstance(deepLinkArg) — i.e., check deepLinkArg and call tryForwardToRunningInstance first, exit the process when it returns true, and only call ProductTelemetry.fire when tryForwardToRunningInstance returned false (or when deepLinkArg is null).core/data/src/commonMain/kotlin/zed/rainxch/core/data/di/SharedModule.kt (1)
167-176:⚠️ Potential issue | 🔴 CriticalBreak the
BackendApiClient↔ProductTelemetryconstructor cycle.
BackendApiClientrequiresProductTelemetry(line 175), whileProductTelemetryImplrequiresBackendApiClient(line 208). Both are Koin singletons, creating a direct cycle that will fail at runtime on first resolution. One side must depend on a narrower/lazy abstraction or defer resolution instead of the concrete singleton.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/data/src/commonMain/kotlin/zed/rainxch/core/data/di/SharedModule.kt` around lines 167 - 176, BackendApiClient and ProductTelemetry form a cyclic Koin singleton dependency (BackendApiClient ctor references ProductTelemetry; ProductTelemetryImpl references BackendApiClient); break the cycle by changing one side to a lazy/provider/functional dependency instead of directly injecting the concrete singleton. For example, modify the ProductTelemetryImpl binding to accept a supplier/provider/lazy reference to BackendApiClient (or accept an interface abstraction) so it calls get() only when needed, or change the BackendApiClient constructor to accept a lightweight telemetry interface rather than ProductTelemetryImpl; update the bindings in SharedModule.kt around the BackendApiClient and ProductTelemetry/ProductTelemetryImpl registrations (while keeping the ProxyManagerSeeding and ProxyScope.DISCOVERY usage) so Koin can construct one without immediately resolving the other.
🧹 Nitpick comments (8)
composeApp/src/commonMain/kotlin/zed/rainxch/githubstore/app/CrashCategory.kt (1)
3-10: Centralize crash category values instead of inline strings.
Using raw literals here increases schema-drift risk and makes refactors harder.♻️ Proposed refactor
+object CrashCategories { + const val OTHER = "other" + const val DATA_LOSS = "data_loss" + const val INSTALL_FAIL = "install_fail" + const val VERSION_DETECT = "version_detect" +} + fun categorizeCrash(throwable: Throwable): String = when { - throwable is OutOfMemoryError -> "other" - throwable.message?.contains("DataStore", ignoreCase = true) == true -> "data_loss" - throwable.message?.contains("install", ignoreCase = true) == true -> "install_fail" - throwable.message?.contains("version", ignoreCase = true) == true -> "version_detect" - else -> "other" + throwable is OutOfMemoryError -> CrashCategories.OTHER + throwable.message?.contains("DataStore", ignoreCase = true) == true -> CrashCategories.DATA_LOSS + throwable.message?.contains("install", ignoreCase = true) == true -> CrashCategories.INSTALL_FAIL + throwable.message?.contains("version", ignoreCase = true) == true -> CrashCategories.VERSION_DETECT + else -> CrashCategories.OTHER }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composeApp/src/commonMain/kotlin/zed/rainxch/githubstore/app/CrashCategory.kt` around lines 3 - 10, The function categorizeCrash currently returns inline string literals ("other", "data_loss", "install_fail", "version_detect"); centralize these into a single source of truth (e.g., an enum or object of constants like CrashCategory { OTHER, DATA_LOSS, INSTALL_FAIL, VERSION_DETECT } or CrashCategory.values as string mappings) and update categorizeCrash to return those constants instead of raw strings; ensure usages across the codebase consume the CrashCategory type or its string representation via a single method/property so the values are decoupled from callers and easy to refactor.core/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/InstalledAppsRepositoryImpl.kt (1)
458-458: Make telemetry emission best-effort inupdateAppVersion.If
productTelemetry.fire(...)throws, this method can report failure after DB writes already committed. Isolate telemetry errors from the persistence path.♻️ Suggested hardening
- productTelemetry.fire(name = ProductTelemetryEvents.UPDATE_INSTALLED) + runCatching { + productTelemetry.fire(name = ProductTelemetryEvents.UPDATE_INSTALLED) + }.onFailure { err -> + Logger.w { "Failed to emit UPDATE_INSTALLED telemetry: ${err.message}" } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/InstalledAppsRepositoryImpl.kt` at line 458, In InstalledAppsRepositoryImpl's updateAppVersion, make the productTelemetry.fire call best-effort by wrapping it in a try/catch so telemetry failures do not affect DB persistence; catch Throwable (or Exception) around productTelemetry.fire(...) and swallow or log the error (do not rethrow) so updateAppVersion returns the persistence result even if telemetry fails.core/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/ProxyRepositoryImpl.kt (1)
189-200: Keep proxy save path resilient to telemetry failures.
setProxyConfighas already persisted and applied config before telemetry. Make telemetry best-effort so a telemetry exception does not surface as a failed save.♻️ Suggested hardening
- productTelemetry.fire( - name = ProductTelemetryEvents.PROXY_CONFIGURED, - props = - mapOf( - ProductTelemetryProps.TYPE to - when (config) { - is ProxyConfig.Http -> "http" - is ProxyConfig.Socks -> "socks5" - ProxyConfig.None, ProxyConfig.System -> "none" - }, - ), - ) + runCatching { + productTelemetry.fire( + name = ProductTelemetryEvents.PROXY_CONFIGURED, + props = + mapOf( + ProductTelemetryProps.TYPE to + when (config) { + is ProxyConfig.Http -> "http" + is ProxyConfig.Socks -> "socks5" + ProxyConfig.None, ProxyConfig.System -> "none" + }, + ), + ) + }.onFailure { err -> + logger.warn("Failed to emit PROXY_CONFIGURED telemetry: ${err.message}") + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/ProxyRepositoryImpl.kt` around lines 189 - 200, The productTelemetry.fire call inside ProxyRepositoryImpl.setProxyConfig is currently executed after the config is persisted/applied but can throw and surface as a failed save; make telemetry best-effort by wrapping the productTelemetry.fire invocation in a try/catch that swallows/logs telemetry errors (do not rethrow) so failures in productTelemetry.fire do not affect the successful completion of setProxyConfig. Ensure you target the productTelemetry.fire call in ProxyRepositoryImpl (the PROXY_CONFIGURED event block) and handle exceptions quietly or log them at debug/trace level.composeApp/src/commonMain/kotlin/zed/rainxch/githubstore/app/navigation/AppNavigation.kt (1)
89-94: Centralize thefromcategories.These literals are now part of the telemetry schema. One typo at any call site will compile cleanly and silently create a new
DETAILS_VIEWEDbucket. Please route them through constants or a typed source enum/sealed value instead of repeating raw strings.Also applies to: 112-126, 146-150, 179-184, 212-217, 234-239, 288-293, 335-340, 356-362
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composeApp/src/commonMain/kotlin/zed/rainxch/githubstore/app/navigation/AppNavigation.kt` around lines 89 - 94, Replace the raw "from" string literals passed into GithubStoreGraph.DetailsScreen across calls like the onNavigateToDetails lambda and the other listed call sites with a centralized typed source (e.g., a sealed class or enum) and a single constant provider; locate usages in the onNavigateToDetails handlers calling GithubStoreGraph.DetailsScreen(repositoryId = repoId, from = "...") and change the parameter to accept/serialize a typed value (or map the typed value to the canonical string once at the navigation boundary) so all call sites use the same identifier (e.g., From.Category or From.SEARCH) rather than literal strings to avoid typos and ensure consistent telemetry buckets.roadmap/E6_CLIENT_HANDOFF.md (1)
11-41: Add language specifiers to fenced code blocks.Static analysis flagged three code blocks missing language specifiers (lines 11, 52, 419). Adding them improves syntax highlighting and markdown linting compliance.
- Line 11: Use
```textfor the file listing- Line 52: Use
```bashfor the grep command- Line 419: Use
```jsonfor the API contractAlso applies to: 52-54, 419-438
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@roadmap/E6_CLIENT_HANDOFF.md` around lines 11 - 41, Update the three fenced code blocks in the markdown: add a language specifier "text" to the file-listing block (the block showing core/... paths and file names), add "bash" to the grep/command block, and add "json" to the API contract block; ensure each opening fence uses the appropriate specifier (```text, ```bash, ```json) and that the corresponding closing ``` remains present so markdown linting and syntax highlighting work correctly.feature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/import/ExternalImportViewModel.kt (1)
690-693: Consider removing the now-empty function and its call sites.
emitPermissionOutcomeis intentionally a no-op per the E6 schema. The call sites at lines 113 and 119 and the function itself could be removed to reduce dead code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/import/ExternalImportViewModel.kt` around lines 690 - 693, Remove the now-empty no-op function emitPermissionOutcome and delete all its call sites where it's invoked with (granted, sdkInt); locate the two calls that currently invoke emitPermissionOutcome and remove those statements, and then delete the emitPermissionOutcome method declaration itself to eliminate dead code and related unused parameters.core/data/src/commonMain/kotlin/zed/rainxch/core/data/telemetry/ProductTelemetryImpl.kt (1)
73-79: Avoid launching coroutines while holding the mutex.The
appScope.launchat line 77 is invoked whilebufferMutexis held. Althoughlaunchis non-blocking, this pattern is a concurrency smell—the mutex guard mixes synchronous buffer mutation with asynchronous flush scheduling.Consider moving the launch outside the lock:
♻️ Proposed refactor to separate concerns
bufferMutex.withLock { if (buffer.size >= MAX_BUFFER_SIZE) buffer.removeFirst() buffer.add(body) - if (buffer.size >= FLUSH_BATCH_THRESHOLD) { - appScope.launch { flushInternal() } - } } + val shouldFlush = bufferMutex.withLock { buffer.size >= FLUSH_BATCH_THRESHOLD } + if (shouldFlush) { + appScope.launch { flushInternal() } + }Note: This requires a second lock acquisition, so alternatively you could return a boolean from the first lock block.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/data/src/commonMain/kotlin/zed/rainxch/core/data/telemetry/ProductTelemetryImpl.kt` around lines 73 - 79, The code currently calls appScope.launch { flushInternal() } while holding bufferMutex which mixes synchronous buffer mutation with async scheduling; modify ProductTelemetryImpl so the lock scope only mutates buffer (using bufferMutex.withLock to handle removeFirst/add) and determine whether a flush is needed inside that lock (e.g., compute a boolean shouldFlush when buffer.size >= FLUSH_BATCH_THRESHOLD), then release the lock and, if shouldFlush, call appScope.launch { flushInternal() } outside the withLock; reference bufferMutex, buffer, MAX_BUFFER_SIZE, FLUSH_BATCH_THRESHOLD, appScope.launch and flushInternal when applying the change.core/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/ExternalImportRepositoryImpl.kt (1)
254-264: Consider using consistent bucket labels withTelemetryBuckets.The
bucketConfidencefunction (lines 556-561) returns numerical range strings ("<0.5","0.5-0.85",">=0.85"), while the handoff doc'sTelemetryBuckets.confidencehelper (section 3.2) uses categorical labels ("low","medium","high").If
TelemetryBucketsis meant to be the centralized bucketing utility, consider using it here for consistency across all telemetry emissions.Also applies to: 556-561
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/ExternalImportRepositoryImpl.kt` around lines 254 - 264, The current telemetry uses bucketConfidence(...) which returns numeric-range strings; replace that with the centralized TelemetryBuckets.confidence(...) (or map bucketConfidence outputs to TelemetryBuckets) so IMPORT_MATCH_ATTEMPTED emits categorical labels used across the codebase; update the call in the deduped.groupBy block (where ProductTelemetryEvents.IMPORT_MATCH_ATTEMPTED and ProductTelemetryProps.CONFIDENCE_BUCKET are set) to pass TelemetryBuckets.confidence(top.confidence) instead of bucketConfidence(top.confidence).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@composeApp/src/androidMain/kotlin/zed/rainxch/githubstore/app/GithubStoreApp.kt`:
- Around line 59-72: The current installSessionDurationObserver uses
ColdStart.elapsedSeconds() and runBlocking in the ON_STOP handler, which reports
growing process uptime as overlapping "session" durations and may block the main
thread; change it to record a distinct foreground-session start time (e.g., set
a foregroundSessionStart variable when lifecycle hits ON_START in the same
LifecycleEventObserver), compute session duration on ON_STOP as now -
foregroundSessionStart instead of ColdStart.elapsedSeconds(), and call
telemetry.fire(...) and telemetry.flush() asynchronously from an application
coroutine scope (appScope) or IO dispatcher rather than using
runBlocking/withTimeoutOrNull in the observer so the main thread is never
blocked. Ensure you still bound the flush with a timeout but perform it from a
background coroutine when invoking telemetry.flush().
In
`@composeApp/src/commonMain/kotlin/zed/rainxch/githubstore/app/navigation/GithubStoreGraph.kt`:
- Around line 22-25: The nav parameter 'from' (declared as val from: String)
allows arbitrary values—replace it with a closed serializable type to enforce
allowed origins; create a `@Serializable` sealed interface or enum (e.g.,
FromOrigin with cases "Search", "Category", "Library", "Link") and update the
route/signature that uses 'from' to accept FromOrigin instead of String (update
any serialization/parsing points and usages in GithubStoreGraph, route builders
and telemetry emission to map to the allowed variants); ensure the sealed type
is used where the 'from' value is constructed and logged so only the approved
entries are produced.
In `@composeApp/src/jvmMain/kotlin/zed/rainxch/githubstore/DesktopApp.kt`:
- Around line 165-181: The custom default uncaught-exception handler currently
calls previous?.uncaughtException(...) but when previous is null it fails to
delegate to the JVM thread-group fallback; update installCrashTelemetryHandler
to call the thread's ThreadGroup uncaughtException when previous is null (e.g.,
if previous != null call previous.uncaughtException(thread, throwable) else call
thread.threadGroup?.uncaughtException(thread, throwable)), preserving existing
telemetry runCatching/flush logic and ensuring the fallback delegation occurs
after telemetry is sent.
In
`@core/data/src/commonMain/kotlin/zed/rainxch/core/data/dto/ProductTelemetryEventBody.kt`:
- Around line 7-13: The data class ProductTelemetryEventBody currently
serializes sessionId and appVersion using camelCase which can mismatch the
backend's snake_case telemetry schema; update the class by adding
kotlinx.serialization `@SerialName` annotations to the sessionId and appVersion
properties (e.g., `@SerialName`("session_id") on sessionId and
`@SerialName`("app_version") on appVersion) so the emitted JSON keys match the
backend contract used by ProductTelemetryImpl.
In
`@core/data/src/commonMain/kotlin/zed/rainxch/core/data/network/BackendApiClient.kt`:
- Around line 310-324: postProductTelemetryEvents is recursively triggering
telemetry because it calls safeCall which in turn calls firePerProxyOutcome
(emitting proxy-used telemetry) and causes another telemetry flush; to fix,
bypass the telemetry-emitting path for telemetry endpoints by adding a
non-telemetry internal code path: modify postProductTelemetryEvents (and the
other telemetry-posting function block at 326-346) to perform the HTTP POST
directly using httpClient.post(...) with contentType and setBody without
wrapping in safeCall (or add a boolean parameter to safeCall like
emitTelemetry=false and pass it when calling from postProductTelemetryEvents),
ensuring safeCall/firePerProxyOutcome are not invoked for the "telemetry/events"
endpoint so no proxy-used telemetry is fired during telemetry flushes.
In
`@core/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/ExternalImportRepositoryImpl.kt`:
- Around line 111-118: The telemetry for IMPORT_AUTO_LINKED is missing; when
auto-linking completes successfully you must fire
ProductTelemetryEvents.IMPORT_AUTO_LINKED with relevant props. Add a
productTelemetry.fire call (similar to the existing IMPORT_SCAN_COMPLETED usage)
at the end of the auto-materialization/auto-linking flow (e.g., in
ExternalImportViewModel or the completion path in ExternalImportRepositoryImpl
where auto-linking finishes), supplying any relevant props such as candidate
count or duration (use ProductTelemetryProps keys) so the event is emitted
consistently with other telemetry.
In
`@core/data/src/commonMain/kotlin/zed/rainxch/core/data/services/DefaultDownloadOrchestrator.kt`:
- Around line 558-577: In markFailed, replace the raw throwable class name
emission with a small, explicit error-bucketing function (e.g.,
mapExceptionToBucket(throwable): String) and emit that bucket in
ProductTelemetryProps.ERROR_CODE; update DefaultDownloadOrchestrator.markFailed
to call your mapper (handle null by returning "unknown") and ensure the mapper
covers the known categories you use elsewhere (network, io, timeout, auth,
parse, unknown) rather than using throwable::class.simpleName so telemetry
remains stable and aggregated.
In
`@core/data/src/commonMain/kotlin/zed/rainxch/core/data/telemetry/ProductTelemetryImpl.kt`:
- Around line 48-56: The runCatching in the appScope.launch loop around
flushInternal() swallows CancellationException and prevents proper coroutine
cancellation; update the failure handling so that if the caught throwable is a
CancellationException it is re-thrown, otherwise log it. Concretely, keep the
appScope.launch and flushInternal() call but replace the onFailure handler to
check `if (it is CancellationException) throw it` before calling
`logger.debug("Product telemetry flush error: ${it.message}")`, referring to the
init block, appScope.launch, flushInternal(), and the runCatching/onFailure
usage.
In
`@core/domain/src/commonMain/kotlin/zed/rainxch/core/domain/telemetry/Buckets.kt`:
- Around line 11-15: The resultCount(n: Int) function currently buckets negative
n into "1-5"; update its when expression to explicitly handle n < 0 first (e.g.,
return "<0" or "invalid") before other checks so invalid upstream data doesn't
end up in the "1-5" bucket; modify the resultCount function in Buckets.kt to add
the n < 0 branch at the top of the when and return the chosen negative-value
label.
In `@core/presentation/src/commonMain/composeResources/values/strings.xml`:
- Around line 692-695: Update the copy for the telemetry strings to stop
claiming “any identifying information” is never collected and instead state that
personal or account-identifying data is not collected; specifically edit
settings_product_telemetry_description and product_telemetry_sheet_body to
replace phrases like “any identifying information” with wording such as
“personal or account identifiers” (or similar), keeping the rest of the
messaging about anonymous, aggregate metrics and open-source schema intact so it
aligns with the resettable analytics/session identifier design.
In
`@feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsViewModel.kt`:
- Around line 2399-2405: The DETAILS_VIEWED telemetry is being emitted every
time loadInitial() runs (e.g., on Retry/refresh); add a one-time guard in
DetailsViewModel so productTelemetry.fire(...) for
zed.rainxch.core.domain.telemetry.ProductTelemetryEvents.DETAILS_VIEWED only
runs once per screen instance: introduce a private boolean flag (e.g.,
detailsViewedFired) on the DetailsViewModel, check it at the start of the
loadInitial() telemetry block, call productTelemetry.fire(...) only if the flag
is false, and set the flag to true immediately after firing to prevent
subsequent Retry/refresh from re-emitting.
In
`@feature/home/presentation/src/commonMain/kotlin/zed/rainxch/home/presentation/HomeConsentGateViewModel.kt`:
- Around line 12-34: HomeConsentGateViewModel currently exposes a raw
repository-backed StateFlow and ad-hoc grant()/deny() methods; refactor it to
the repo-wide State/Action/Event pattern by introducing a private
MutableStateFlow (e.g., _consentState) and a public StateFlow (consentState), a
sealed interface Action with actions like GrantConsent and DenyConsent, and a
public handle(action: Action) method that routes actions through
viewModelScope.launch to call tweaksRepository.setProductTelemetryConsent(...)
and update _consentState; also add a Channel<Event> (or MutableSharedFlow) for
one-off events and emit success/error events when repository calls complete, and
initialize the private state by collecting
tweaksRepository.getProductTelemetryConsent() into _consentState in
viewModelScope.
- Around line 16-21: The consent StateFlow in HomeConsentGateViewModel is seeded
with ProductTelemetryConsent.NotYetAsked (consent), which makes the UI treat a
loading state as “not answered”; change the flow to emit a distinct loading/null
state instead of NotYetAsked by making consent a
StateFlow<ProductTelemetryConsent?> (or introduce a Loading variant) and call
tweaksRepository.getProductTelemetryConsent().stateIn(...) with initialValue =
null (or Loading) so the first real value from getProductTelemetryConsent() is
respected; update HomeRoot (or any UI consumer) to treat null/Loading as “still
loading” and only show the consent sheet when the value is explicitly
NotYetAsked.
In
`@feature/home/presentation/src/commonMain/kotlin/zed/rainxch/home/presentation/HomeRoot.kt`:
- Around line 111-112: The TELEMETRY_SCHEMA_URL constant in HomeRoot.kt
currently points at a mutable main branch URL; change this to an immutable
reference by either (A) replacing the value with a URL pinned to a specific
release tag or commit hash (e.g.,
https://github.com/.../blob/<tag-or-commit>/...TelemetryEvent.kt) or (B) bundle
the schema locally and update the code to load the consent schema from a local
resource (e.g., add a telemetry_schema.txt/kt resource and replace
TELEMETRY_SCHEMA_URL with a local loader/constant like TELEMETRY_SCHEMA_TEXT),
ensuring the shipped client always displays the exact schema shipped with that
build.
---
Outside diff comments:
In `@composeApp/src/jvmMain/kotlin/zed/rainxch/githubstore/DesktopApp.kt`:
- Around line 39-45: Move the CrashReporter.install() call to be the very first
statement inside the main function so stdout/stderr are tee'd before anything
else; specifically, in DesktopApp.main swap the order of CrashReporter.install()
and ColdStart.markStart() (call CrashReporter.install() first, then
ColdStart.markStart()) so any failures during ColdStart.markStart() are captured
by the crash/session logging.
- Around line 83-97: The APP_LAUNCHED telemetry is sent before checking
deep-link handoff, causing transient handoff processes to be counted as real
launches; modify the logic so that ProductTelemetry.fire(...) for
ProductTelemetryEvents.APP_LAUNCHED (the call currently passing props =
mapOf(ProductTelemetryProps.PLATFORM to desktopPlatformSlug())) only runs if no
deepLinkArg was forwarded by
DesktopDeepLink.tryForwardToRunningInstance(deepLinkArg) — i.e., check
deepLinkArg and call tryForwardToRunningInstance first, exit the process when it
returns true, and only call ProductTelemetry.fire when
tryForwardToRunningInstance returned false (or when deepLinkArg is null).
In
`@core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/UpdateCheckWorker.kt`:
- Around line 94-107: The catch block in UpdateCheckWorker is converting
coroutine CancellationException into an OPERATION_FAILED telemetry event; update
the handler so CancellationException is not treated as a regular failure: inside
the catch for Exception in the UpdateCheckWorker (the block that logs the error,
checks runAttemptCount, calls Result.retry()/Result.failure(), and fires
productTelemetry.fire), detect if e is a CancellationException (or inherits from
Throwable that indicates cancellation) and rethrow it immediately (or return
Result.retry/propagate cancellation) so it is not logged as OPERATION_FAILED;
only call productTelemetry.fire for non-cancellation exceptions when
runAttemptCount >= 3 and then return Result.failure(). Ensure you reference the
same symbols: UpdateCheckWorker, runAttemptCount, productTelemetry.fire,
ProductTelemetryEvents.OPERATION_FAILED, and ProductTelemetryProps.
In `@core/data/src/commonMain/kotlin/zed/rainxch/core/data/di/SharedModule.kt`:
- Around line 167-176: BackendApiClient and ProductTelemetry form a cyclic Koin
singleton dependency (BackendApiClient ctor references ProductTelemetry;
ProductTelemetryImpl references BackendApiClient); break the cycle by changing
one side to a lazy/provider/functional dependency instead of directly injecting
the concrete singleton. For example, modify the ProductTelemetryImpl binding to
accept a supplier/provider/lazy reference to BackendApiClient (or accept an
interface abstraction) so it calls get() only when needed, or change the
BackendApiClient constructor to accept a lightweight telemetry interface rather
than ProductTelemetryImpl; update the bindings in SharedModule.kt around the
BackendApiClient and ProductTelemetry/ProductTelemetryImpl registrations (while
keeping the ProxyManagerSeeding and ProxyScope.DISCOVERY usage) so Koin can
construct one without immediately resolving the other.
---
Nitpick comments:
In
`@composeApp/src/commonMain/kotlin/zed/rainxch/githubstore/app/CrashCategory.kt`:
- Around line 3-10: The function categorizeCrash currently returns inline string
literals ("other", "data_loss", "install_fail", "version_detect"); centralize
these into a single source of truth (e.g., an enum or object of constants like
CrashCategory { OTHER, DATA_LOSS, INSTALL_FAIL, VERSION_DETECT } or
CrashCategory.values as string mappings) and update categorizeCrash to return
those constants instead of raw strings; ensure usages across the codebase
consume the CrashCategory type or its string representation via a single
method/property so the values are decoupled from callers and easy to refactor.
In
`@composeApp/src/commonMain/kotlin/zed/rainxch/githubstore/app/navigation/AppNavigation.kt`:
- Around line 89-94: Replace the raw "from" string literals passed into
GithubStoreGraph.DetailsScreen across calls like the onNavigateToDetails lambda
and the other listed call sites with a centralized typed source (e.g., a sealed
class or enum) and a single constant provider; locate usages in the
onNavigateToDetails handlers calling GithubStoreGraph.DetailsScreen(repositoryId
= repoId, from = "...") and change the parameter to accept/serialize a typed
value (or map the typed value to the canonical string once at the navigation
boundary) so all call sites use the same identifier (e.g., From.Category or
From.SEARCH) rather than literal strings to avoid typos and ensure consistent
telemetry buckets.
In
`@core/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/ExternalImportRepositoryImpl.kt`:
- Around line 254-264: The current telemetry uses bucketConfidence(...) which
returns numeric-range strings; replace that with the centralized
TelemetryBuckets.confidence(...) (or map bucketConfidence outputs to
TelemetryBuckets) so IMPORT_MATCH_ATTEMPTED emits categorical labels used across
the codebase; update the call in the deduped.groupBy block (where
ProductTelemetryEvents.IMPORT_MATCH_ATTEMPTED and
ProductTelemetryProps.CONFIDENCE_BUCKET are set) to pass
TelemetryBuckets.confidence(top.confidence) instead of
bucketConfidence(top.confidence).
In
`@core/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/InstalledAppsRepositoryImpl.kt`:
- Line 458: In InstalledAppsRepositoryImpl's updateAppVersion, make the
productTelemetry.fire call best-effort by wrapping it in a try/catch so
telemetry failures do not affect DB persistence; catch Throwable (or Exception)
around productTelemetry.fire(...) and swallow or log the error (do not rethrow)
so updateAppVersion returns the persistence result even if telemetry fails.
In
`@core/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/ProxyRepositoryImpl.kt`:
- Around line 189-200: The productTelemetry.fire call inside
ProxyRepositoryImpl.setProxyConfig is currently executed after the config is
persisted/applied but can throw and surface as a failed save; make telemetry
best-effort by wrapping the productTelemetry.fire invocation in a try/catch that
swallows/logs telemetry errors (do not rethrow) so failures in
productTelemetry.fire do not affect the successful completion of setProxyConfig.
Ensure you target the productTelemetry.fire call in ProxyRepositoryImpl (the
PROXY_CONFIGURED event block) and handle exceptions quietly or log them at
debug/trace level.
In
`@core/data/src/commonMain/kotlin/zed/rainxch/core/data/telemetry/ProductTelemetryImpl.kt`:
- Around line 73-79: The code currently calls appScope.launch { flushInternal()
} while holding bufferMutex which mixes synchronous buffer mutation with async
scheduling; modify ProductTelemetryImpl so the lock scope only mutates buffer
(using bufferMutex.withLock to handle removeFirst/add) and determine whether a
flush is needed inside that lock (e.g., compute a boolean shouldFlush when
buffer.size >= FLUSH_BATCH_THRESHOLD), then release the lock and, if
shouldFlush, call appScope.launch { flushInternal() } outside the withLock;
reference bufferMutex, buffer, MAX_BUFFER_SIZE, FLUSH_BATCH_THRESHOLD,
appScope.launch and flushInternal when applying the change.
In
`@feature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/import/ExternalImportViewModel.kt`:
- Around line 690-693: Remove the now-empty no-op function emitPermissionOutcome
and delete all its call sites where it's invoked with (granted, sdkInt); locate
the two calls that currently invoke emitPermissionOutcome and remove those
statements, and then delete the emitPermissionOutcome method declaration itself
to eliminate dead code and related unused parameters.
In `@roadmap/E6_CLIENT_HANDOFF.md`:
- Around line 11-41: Update the three fenced code blocks in the markdown: add a
language specifier "text" to the file-listing block (the block showing core/...
paths and file names), add "bash" to the grep/command block, and add "json" to
the API contract block; ensure each opening fence uses the appropriate specifier
(```text, ```bash, ```json) and that the corresponding closing ``` remains
present so markdown linting and syntax highlighting work correctly.
🪄 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: 01c751d5-e1da-4f55-933b-c31ea9dadb05
📒 Files selected for processing (45)
composeApp/build.gradle.ktscomposeApp/src/androidMain/kotlin/zed/rainxch/githubstore/app/GithubStoreApp.ktcomposeApp/src/commonMain/kotlin/zed/rainxch/githubstore/Main.ktcomposeApp/src/commonMain/kotlin/zed/rainxch/githubstore/app/ColdStart.ktcomposeApp/src/commonMain/kotlin/zed/rainxch/githubstore/app/CrashCategory.ktcomposeApp/src/commonMain/kotlin/zed/rainxch/githubstore/app/di/ViewModelsModule.ktcomposeApp/src/commonMain/kotlin/zed/rainxch/githubstore/app/navigation/AppNavigation.ktcomposeApp/src/commonMain/kotlin/zed/rainxch/githubstore/app/navigation/GithubStoreGraph.ktcomposeApp/src/jvmMain/kotlin/zed/rainxch/githubstore/DesktopApp.ktcore/data/src/androidMain/kotlin/zed/rainxch/core/data/services/UpdateCheckWorker.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/cache/CacheManager.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/di/SharedModule.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/dto/ProductTelemetryEventBody.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/network/BackendApiClient.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/ExternalImportRepositoryImpl.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/InstalledAppsRepositoryImpl.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/ProxyRepositoryImpl.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/TweaksRepositoryImpl.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/services/DefaultDownloadOrchestrator.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/telemetry/ProductTelemetryImpl.ktcore/domain/build.gradle.ktscore/domain/src/commonMain/kotlin/zed/rainxch/core/domain/repository/TweaksRepository.ktcore/domain/src/commonMain/kotlin/zed/rainxch/core/domain/telemetry/Buckets.ktcore/domain/src/commonMain/kotlin/zed/rainxch/core/domain/telemetry/ProductTelemetry.ktcore/domain/src/commonMain/kotlin/zed/rainxch/core/domain/telemetry/ProductTelemetryConsent.ktcore/domain/src/commonMain/kotlin/zed/rainxch/core/domain/telemetry/ProductTelemetryEvents.ktcore/domain/src/commonTest/kotlin/zed/rainxch/core/domain/telemetry/PrivacyAuditTest.ktcore/presentation/src/commonMain/composeResources/values/strings.xmlcore/presentation/src/commonMain/kotlin/zed/rainxch/core/presentation/telemetry/TrackFirstPaint.ktfeature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/AppsRoot.ktfeature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/import/ExternalImportViewModel.ktfeature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsRoot.ktfeature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsViewModel.ktfeature/home/presentation/src/commonMain/kotlin/zed/rainxch/home/presentation/HomeConsentGateViewModel.ktfeature/home/presentation/src/commonMain/kotlin/zed/rainxch/home/presentation/HomeRoot.ktfeature/home/presentation/src/commonMain/kotlin/zed/rainxch/home/presentation/components/ProductTelemetryConsentSheet.ktfeature/search/presentation/src/commonMain/kotlin/zed/rainxch/search/presentation/SearchRoot.ktfeature/search/presentation/src/commonMain/kotlin/zed/rainxch/search/presentation/SearchViewModel.ktfeature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/TweaksAction.ktfeature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/TweaksRoot.ktfeature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/TweaksState.ktfeature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/TweaksViewModel.ktfeature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/components/sections/Others.ktgradle/libs.versions.tomlroadmap/E6_CLIENT_HANDOFF.md
Comment on lines
+7
to
+13
| data class ProductTelemetryEventBody( | ||
| val name: String, | ||
| val sessionId: String, | ||
| val timestamp: Long, | ||
| val platform: String? = null, | ||
| val appVersion: String? = null, | ||
| val props: JsonObject? = null, |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n --glob '*.kt' 'JsonNamingStrategy|namingStrategy|snake.?case|kotlinx\.serialization\.json\.Json|ContentNegotiation'
rg -n --glob '*.kt' '@SerialName\("session_id"\)|@SerialName\("app_version"\)'Repository: OpenHub-Store/GitHub-Store
Length of output: 3306
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get HttpClientFactory.kt around ContentNegotiation setup
echo "=== HttpClientFactory.kt ContentNegotiation setup ==="
sed -n '50,60p' core/data/src/commonMain/kotlin/zed/rainxch/core/data/network/HttpClientFactory.kt
# Get BackendApiClient.kt around ContentNegotiation setup
echo -e "\n=== BackendApiClient.kt ContentNegotiation setup ==="
sed -n '85,95p' core/data/src/commonMain/kotlin/zed/rainxch/core/data/network/BackendApiClient.kt
# Check entire ProductTelemetryEventBody.kt
echo -e "\n=== ProductTelemetryEventBody.kt (full file) ==="
cat -n core/data/src/commonMain/kotlin/zed/rainxch/core/data/dto/ProductTelemetryEventBody.kt
# Check if there's a centralized Json configuration
echo -e "\n=== Check for Json factory/helper files ==="
find core/data -name "*.kt" -type f | xargs grep -l "fun.*Json\|object.*Json" | head -10Repository: OpenHub-Store/GitHub-Store
Length of output: 1629
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read full HttpClientFactory.kt to find json variable
echo "=== Full HttpClientFactory.kt ==="
cat -n core/data/src/commonMain/kotlin/zed/rainxch/core/data/network/HttpClientFactory.kt
echo -e "\n=== ProductTelemetryImpl.kt to see telemetry sending ==="
cat -n core/data/src/commonMain/kotlin/zed/rainxch/core/data/telemetry/ProductTelemetryImpl.kt | head -100
# Check if BackendApiClient is used for telemetry
echo -e "\n=== BackendApiClient usage in telemetry ==="
grep -n "BackendApiClient\|HttpClientFactory" core/data/src/commonMain/kotlin/zed/rainxch/core/data/telemetry/ProductTelemetryImpl.ktRepository: OpenHub-Store/GitHub-Store
Length of output: 9651
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check PrivacyAuditTest to see field naming tests
echo "=== PrivacyAuditTest.kt ==="
cat -n core/domain/src/commonTest/kotlin/zed/rainxch/core/domain/telemetry/PrivacyAuditTest.kt | head -60
# Check for telemetry API endpoint documentation
echo -e "\n=== Search for telemetry endpoint references ==="
rg -n "session_id|sessionId|app_version|appVersion" --glob "*.kt" | grep -i "telemetry\|event" | head -20
# Check BackendApiClient for telemetry endpoint
echo -e "\n=== BackendApiClient telemetry endpoints ==="
grep -n -A 5 "telemetry\|events" core/data/src/commonMain/kotlin/zed/rainxch/core/data/network/BackendApiClient.kt
# Check for any Json configuration in BackendApiClient
echo -e "\n=== Full BackendApiClient.kt ==="
cat -n core/data/src/commonMain/kotlin/zed/rainxch/core/data/network/BackendApiClient.ktRepository: OpenHub-Store/GitHub-Store
Length of output: 20872
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if there are any backend contract tests or documentation
echo "=== Search for E6 or backend contract docs ==="
find . -type f \( -name "*.md" -o -name "*.txt" -o -name "*.kt" \) | xargs grep -l "E6\|telemetry.*contract\|session_id.*field" 2>/dev/null | head -10
# Check EventRequest to see how it handles naming
echo -e "\n=== EventRequest.kt (similar DTO for events endpoint) ==="
cat -n core/data/src/commonMain/kotlin/zed/rainxch/core/data/dto/EventRequest.kt
# Check if there are any other telemetry-related tests
echo -e "\n=== Search for telemetry tests ==="
find . -path "*/test*" -name "*.kt" -type f | xargs grep -l "telemetry\|ProductTelemetryEventBody" 2>/dev/null
# Check ProductTelemetryImpl fully around the flush to see how batch is sent
echo -e "\n=== ProductTelemetryImpl.kt around flushInternal (lines 90-120) ==="
sed -n '90,120p' core/data/src/commonMain/kotlin/zed/rainxch/core/data/telemetry/ProductTelemetryImpl.ktRepository: OpenHub-Store/GitHub-Store
Length of output: 2326
🏁 Script executed:
#!/bin/bash
set -euo pipefixpipeline
# Get the E6 contract comment from ProductTelemetryImpl
echo "=== ProductTelemetryImpl.kt E6 contract reference (lines 36-42) ==="
sed -n '36,42p' core/data/src/commonMain/kotlin/zed/rainxch/core/data/telemetry/ProductTelemetryImpl.kt
# Search more thoroughly for JsonNamingStrategy or Json builder with module
echo -e "\n=== Search for Json builders and naming strategies ==="
rg -n "Json\s*\{" --glob "*.kt" -A 5 | head -80
# Check TelemetryRepositoryImpl for similar patterns
echo -e "\n=== TelemetryRepositoryImpl.kt (first 50 lines and fields) ==="
head -60 core/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/TelemetryRepositoryImpl.kt
# Check ProductTelemetry.kt comment about snake_case
echo -e "\n=== ProductTelemetry.kt ==="
cat -n core/domain/src/commonMain/kotlin/zed/rainxch/core/domain/telemetry/ProductTelemetry.kt | head -30Repository: OpenHub-Store/GitHub-Store
Length of output: 131
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get the E6 contract comment from ProductTelemetryImpl
echo "=== ProductTelemetryImpl.kt E6 contract reference (lines 36-42) ==="
sed -n '36,42p' core/data/src/commonMain/kotlin/zed/rainxch/core/data/telemetry/ProductTelemetryImpl.kt
# Search more thoroughly for JsonNamingStrategy or Json builder with module
echo -e "\n=== Search for Json builders and naming strategies ==="
rg -n "Json\s*\{" --glob "*.kt" -A 5 | head -80
# Check TelemetryRepositoryImpl for similar patterns
echo -e "\n=== TelemetryRepositoryImpl.kt (first 50 lines and fields) ==="
head -60 core/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/TelemetryRepositoryImpl.kt
# Check ProductTelemetry.kt comment about snake_case
echo -e "\n=== ProductTelemetry.kt ==="
cat -n core/domain/src/commonMain/kotlin/zed/rainxch/core/domain/telemetry/ProductTelemetry.kt | head -30Repository: OpenHub-Store/GitHub-Store
Length of output: 9728
Add @SerialName annotations to match the backend field casing.
kotlinx.serialization will emit sessionId and appVersion here unless the Json instance applies a snake_case naming strategy. No global snake_case strategy is configured in this codebase. The E6 telemetry contract (as referenced in ProductTelemetryImpl) targets POST /v1/telemetry/events with a locked schema. If the backend expects snake_case field names (session_id, app_version), this payload will be rejected at validation. Annotate both fields explicitly:
Required fix
import kotlinx.serialization.Serializable
+import kotlinx.serialization.SerialName
import kotlinx.serialization.json.JsonObject
`@Serializable`
data class ProductTelemetryEventBody(
val name: String,
+ `@SerialName`("session_id")
val sessionId: String,
val timestamp: Long,
val platform: String? = null,
+ `@SerialName`("app_version")
val appVersion: String? = null,
val props: JsonObject? = null,
)📝 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.
Suggested change
| data class ProductTelemetryEventBody( | |
| val name: String, | |
| val sessionId: String, | |
| val timestamp: Long, | |
| val platform: String? = null, | |
| val appVersion: String? = null, | |
| val props: JsonObject? = null, | |
| import kotlinx.serialization.Serializable | |
| import kotlinx.serialization.SerialName | |
| import kotlinx.serialization.json.JsonObject | |
| `@Serializable` | |
| data class ProductTelemetryEventBody( | |
| val name: String, | |
| `@SerialName`("session_id") | |
| val sessionId: String, | |
| val timestamp: Long, | |
| val platform: String? = null, | |
| `@SerialName`("app_version") | |
| val appVersion: String? = null, | |
| val props: JsonObject? = null, | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@core/data/src/commonMain/kotlin/zed/rainxch/core/data/dto/ProductTelemetryEventBody.kt`
around lines 7 - 13, The data class ProductTelemetryEventBody currently
serializes sessionId and appVersion using camelCase which can mismatch the
backend's snake_case telemetry schema; update the class by adding
kotlinx.serialization `@SerialName` annotations to the sessionId and appVersion
properties (e.g., `@SerialName`("session_id") on sessionId and
`@SerialName`("app_version") on appVersion) so the emitted JSON keys match the
backend contract used by ProductTelemetryImpl.
Comment on lines
+12
to
+34
| class HomeConsentGateViewModel( | ||
| private val tweaksRepository: TweaksRepository, | ||
| ) : ViewModel() { | ||
|
|
||
| val consent: StateFlow<ProductTelemetryConsent> = | ||
| tweaksRepository.getProductTelemetryConsent().stateIn( | ||
| scope = viewModelScope, | ||
| started = SharingStarted.WhileSubscribed(5_000L), | ||
| initialValue = ProductTelemetryConsent.NotYetAsked, | ||
| ) | ||
|
|
||
| fun grant() { | ||
| viewModelScope.launch { | ||
| tweaksRepository.setProductTelemetryConsent(ProductTelemetryConsent.Granted) | ||
| } | ||
| } | ||
|
|
||
| fun deny() { | ||
| viewModelScope.launch { | ||
| tweaksRepository.setProductTelemetryConsent(ProductTelemetryConsent.Denied) | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Align this ViewModel with the repo’s State/Action/Event pattern.
This introduces an ad-hoc ViewModel shape (grant() / deny() methods plus a raw repository-backed flow) instead of the standard presentation contract used elsewhere. Wrapping consent in state and routing user input through actions will keep the home presentation layer consistent.
As per coding guidelines, "Implement State/Action/Event pattern for all ViewModels: state in MutableStateFlow, actions as sealed interface, events in Channel."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@feature/home/presentation/src/commonMain/kotlin/zed/rainxch/home/presentation/HomeConsentGateViewModel.kt`
around lines 12 - 34, HomeConsentGateViewModel currently exposes a raw
repository-backed StateFlow and ad-hoc grant()/deny() methods; refactor it to
the repo-wide State/Action/Event pattern by introducing a private
MutableStateFlow (e.g., _consentState) and a public StateFlow (consentState), a
sealed interface Action with actions like GrantConsent and DenyConsent, and a
public handle(action: Action) method that routes actions through
viewModelScope.launch to call tweaksRepository.setProductTelemetryConsent(...)
and update _consentState; also add a Channel<Event> (or MutableSharedFlow) for
one-off events and emit success/error events when repository calls complete, and
initialize the private state by collecting
tweaksRepository.getProductTelemetryConsent() into _consentState in
viewModelScope.
…nException, narrow buffer-mutex scope
…H falls back to ThreadGroup
…APP_LAUNCHED, UEH falls back to ThreadGroup
…ches the running build
…ckets.confidence, drop dead emitPermissionOutcome
…hey can't break persistence
…fying information'
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
core/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/ExternalImportRepositoryImpl.kt (2)
112-119: Telemetry emission not wrapped in best-effort pattern.Unlike
scheduleInitialScanIfNeeded(which wraps the outer logic inrunCatching), and other telemetry calls in this PR (e.g.,InstalledAppsRepositoryImpl.updateAppVersion), theIMPORT_SCAN_COMPLETEDemission here is not guarded. IfproductTelemetry.firethrows, it could fail the scan operation.Consider wrapping for consistency with the PR's "best-effort telemetry" pattern:
🛡️ Suggested defensive wrap
- productTelemetry.fire( - name = ProductTelemetryEvents.IMPORT_SCAN_COMPLETED, - props = - mapOf( - ProductTelemetryProps.CANDIDATE_COUNT to bucketCandidateCount(candidates.size), - ProductTelemetryProps.DURATION_MS to bucketDurationMs(durationMs), - ), - ) + runCatching { + productTelemetry.fire( + name = ProductTelemetryEvents.IMPORT_SCAN_COMPLETED, + props = + mapOf( + ProductTelemetryProps.CANDIDATE_COUNT to bucketCandidateCount(candidates.size), + ProductTelemetryProps.DURATION_MS to bucketDurationMs(durationMs), + ), + ) + }.onFailure { Logger.d { "IMPORT_SCAN_COMPLETED telemetry failed: ${it.message}" } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/ExternalImportRepositoryImpl.kt` around lines 112 - 119, Wrap the productTelemetry.fire call for IMPORT_SCAN_COMPLETED in a best-effort try/catch (or runCatching) so telemetry failures can't bubble up and fail the scan; specifically surround the productTelemetry.fire(...) invocation (the call that uses ProductTelemetryEvents.IMPORT_SCAN_COMPLETED with props built via bucketCandidateCount and bucketDurationMs) with a non-throwing guard and swallow or log the exception via the repository's logger instead of letting it propagate. Ensure the surrounding method in ExternalImportRepositoryImpl continues to return/complete normally even if telemetry.fire throws.
257-265: Same best-effort pattern missing forIMPORT_MATCH_ATTEMPTED.This telemetry call inside the loop is also unguarded. A failure here could interrupt match resolution for remaining candidates.
🛡️ Suggested defensive wrap
deduped.groupBy { it.source }.forEach { (source, hits) -> val top = hits.maxByOrNull { it.confidence } ?: return@forEach - productTelemetry.fire( - name = ProductTelemetryEvents.IMPORT_MATCH_ATTEMPTED, - props = - mapOf( - ProductTelemetryProps.STRATEGY to source.telemetryStrategy(), - ProductTelemetryProps.CONFIDENCE_BUCKET to - TelemetryBuckets.confidence(top.confidence.toFloat()), - ), - ) + runCatching { + productTelemetry.fire( + name = ProductTelemetryEvents.IMPORT_MATCH_ATTEMPTED, + props = + mapOf( + ProductTelemetryProps.STRATEGY to source.telemetryStrategy(), + ProductTelemetryProps.CONFIDENCE_BUCKET to + TelemetryBuckets.confidence(top.confidence.toFloat()), + ), + ) + }.onFailure { Logger.d { "IMPORT_MATCH_ATTEMPTED telemetry failed: ${it.message}" } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/ExternalImportRepositoryImpl.kt` around lines 257 - 265, The productTelemetry.fire call for ProductTelemetryEvents.IMPORT_MATCH_ATTEMPTED is unguarded and should be wrapped in the same best-effort defensive pattern used elsewhere: catch and ignore/log any exception so telemetry failures don't abort the match loop. Locate the call to productTelemetry.fire with ProductTelemetryEvents.IMPORT_MATCH_ATTEMPTED (using props built from source.telemetryStrategy() and TelemetryBuckets.confidence(top.confidence.toFloat())) and wrap it in a try/catch (or runCatching) that logs or swallows errors, ensuring the loop continues on telemetry failure.composeApp/src/androidMain/kotlin/zed/rainxch/githubstore/app/GithubStoreApp.kt (1)
129-133: Consider API 33+ deprecation ofgetPackageInfo(String, Int).
PackageManager.getPackageInfo(packageName, 0)is deprecated on API 33+. While it still works, you may want to use thePackageInfoFlagsvariant for forward compatibility:♻️ Optional fix for API 33+
- packageManager - .getPackageInfo(packageName, 0) - .versionName - .orEmpty(), + if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.TIRAMISU) { + packageManager + .getPackageInfo(packageName, PackageManager.PackageInfoFlags.of(0)) + .versionName + .orEmpty() + } else { + `@Suppress`("DEPRECATION") + packageManager + .getPackageInfo(packageName, 0) + .versionName + .orEmpty() + },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composeApp/src/androidMain/kotlin/zed/rainxch/githubstore/app/GithubStoreApp.kt` around lines 129 - 133, The code uses the deprecated PackageManager.getPackageInfo(packageName, 0) when populating ProductTelemetryProps.VERSION; replace that call with the API-33-friendly overload by passing PackageInfoFlags (e.g., PackageManager.PackageInfoFlags.of(0)) so the call becomes packageManager.getPackageInfo(packageName, PackageInfoFlags.of(0)) to maintain forward compatibility with API 33+ while still obtaining versionName.core/data/src/commonMain/kotlin/zed/rainxch/core/data/telemetry/ProductTelemetryImpl.kt (1)
89-91: Concurrent flush calls possible.The threshold-triggered flush at line 90 can run concurrently with the periodic flush loop from
init. While buffer access is mutex-protected, this may cause:
- Multiple simultaneous network requests
- Minor race condition on
backoffMsread-modify-write at line 135Consider adding a flush-in-progress guard to coalesce concurrent flush attempts.
♻️ Proposed fix using AtomicBoolean guard
+ private val flushInProgress = java.util.concurrent.atomic.AtomicBoolean(false) + private suspend fun flushInternal() { + if (!flushInProgress.compareAndSet(false, true)) return + try { if (consent() != ProductTelemetryConsent.Granted) { // ...existing logic... } // ...rest of flushInternal... + } finally { + flushInProgress.set(false) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/data/src/commonMain/kotlin/zed/rainxch/core/data/telemetry/ProductTelemetryImpl.kt` around lines 89 - 91, Multiple flushes can run concurrently (threshold-triggered via appScope.launch and the periodic flush loop in init), causing duplicate network requests and races on backoffMs; add a flush-in-progress guard (e.g., an AtomicBoolean named isFlushing) checked and set at the start of flushInternal() and cleared at the end so concurrent callers (including the appScope.launch path that invokes flushInternal()) coalesce, and ensure backoffMs read-modify-write happens only while the guard is held to avoid races.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@core/data/src/commonMain/kotlin/zed/rainxch/core/data/network/BackendApiClient.kt`:
- Around line 333-352: safeCall currently reads currentProxyConfig after block()
returns, which can misattribute or drop PROXY_USED events; snapshot the proxy
configuration before invoking block() and use that snapshot when firing
telemetry. Concretely, in safeCall() capture val proxySnapshot =
currentProxyConfig before calling block(), then change firePerProxyOutcome to
accept a proxy parameter (or add a private overload) and call
firePerProxyOutcome(proxySnapshot, result.isSuccess) so the telemetry uses the
proxySnapshot and not the live currentProxyConfig; reference safeCall,
firePerProxyOutcome, currentProxyConfig, ProxyConfig.None/ProxyConfig.System,
productTelemetry and ProductTelemetryEvents.PROXY_USED.
In
`@core/data/src/commonMain/kotlin/zed/rainxch/core/data/telemetry/ProductTelemetryImpl.kt`:
- Around line 144-146: The helper consent() in ProductTelemetryImpl currently
uses runCatching which swallows CancellationException and delays coroutine
cancellation; update consent() (the function calling
tweaksRepository.getProductTelemetryConsent().first()) to rethrow
CancellationException instead of swallowing it — e.g. use a try/catch around the
call or check the thrown exception from runCatching and if it is a
CancellationException rethrow it, otherwise return
ProductTelemetryConsent.NotYetAsked as the default.
In
`@feature/home/presentation/src/commonMain/kotlin/zed/rainxch/home/presentation/HomeRoot.kt`:
- Around line 138-147: HomeState currently defaults isLoading=false so the first
.stateIn(initialValue = HomeState()) emission makes TrackFirstPaint(isReady =
!state.isLoading) fire immediately and record near-zero paint; fix by either
initializing HomeState with isLoading = true or change the readiness predicate
passed to TrackFirstPaint to a stronger signal (for example, check that repos
are loaded and non-empty or a dedicated isReady flag set after
loadRepos(isInitial = true) completes) — update the HomeState constructor or the
TrackFirstPaint call accordingly and ensure loadRepos(isInitial = true) clears
that loading flag when done.
---
Nitpick comments:
In
`@composeApp/src/androidMain/kotlin/zed/rainxch/githubstore/app/GithubStoreApp.kt`:
- Around line 129-133: The code uses the deprecated
PackageManager.getPackageInfo(packageName, 0) when populating
ProductTelemetryProps.VERSION; replace that call with the API-33-friendly
overload by passing PackageInfoFlags (e.g.,
PackageManager.PackageInfoFlags.of(0)) so the call becomes
packageManager.getPackageInfo(packageName, PackageInfoFlags.of(0)) to maintain
forward compatibility with API 33+ while still obtaining versionName.
In
`@core/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/ExternalImportRepositoryImpl.kt`:
- Around line 112-119: Wrap the productTelemetry.fire call for
IMPORT_SCAN_COMPLETED in a best-effort try/catch (or runCatching) so telemetry
failures can't bubble up and fail the scan; specifically surround the
productTelemetry.fire(...) invocation (the call that uses
ProductTelemetryEvents.IMPORT_SCAN_COMPLETED with props built via
bucketCandidateCount and bucketDurationMs) with a non-throwing guard and swallow
or log the exception via the repository's logger instead of letting it
propagate. Ensure the surrounding method in ExternalImportRepositoryImpl
continues to return/complete normally even if telemetry.fire throws.
- Around line 257-265: The productTelemetry.fire call for
ProductTelemetryEvents.IMPORT_MATCH_ATTEMPTED is unguarded and should be wrapped
in the same best-effort defensive pattern used elsewhere: catch and ignore/log
any exception so telemetry failures don't abort the match loop. Locate the call
to productTelemetry.fire with ProductTelemetryEvents.IMPORT_MATCH_ATTEMPTED
(using props built from source.telemetryStrategy() and
TelemetryBuckets.confidence(top.confidence.toFloat())) and wrap it in a
try/catch (or runCatching) that logs or swallows errors, ensuring the loop
continues on telemetry failure.
In
`@core/data/src/commonMain/kotlin/zed/rainxch/core/data/telemetry/ProductTelemetryImpl.kt`:
- Around line 89-91: Multiple flushes can run concurrently (threshold-triggered
via appScope.launch and the periodic flush loop in init), causing duplicate
network requests and races on backoffMs; add a flush-in-progress guard (e.g., an
AtomicBoolean named isFlushing) checked and set at the start of flushInternal()
and cleared at the end so concurrent callers (including the appScope.launch path
that invokes flushInternal()) coalesce, and ensure backoffMs read-modify-write
happens only while the guard is held to avoid races.
🪄 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: e5e3c8c1-34fb-47fd-90cb-fbc3b5b683f1
📒 Files selected for processing (19)
composeApp/src/androidMain/kotlin/zed/rainxch/githubstore/app/GithubStoreApp.ktcomposeApp/src/commonMain/kotlin/zed/rainxch/githubstore/app/navigation/AppNavigation.ktcomposeApp/src/commonMain/kotlin/zed/rainxch/githubstore/app/navigation/GithubStoreGraph.ktcomposeApp/src/jvmMain/kotlin/zed/rainxch/githubstore/DesktopApp.ktcore/data/src/androidMain/kotlin/zed/rainxch/core/data/services/UpdateCheckWorker.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/di/SharedModule.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/network/BackendApiClient.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/ExternalImportRepositoryImpl.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/InstalledAppsRepositoryImpl.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/ProxyRepositoryImpl.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/services/DefaultDownloadOrchestrator.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/telemetry/ProductTelemetryImpl.ktcore/domain/src/commonMain/kotlin/zed/rainxch/core/domain/telemetry/Buckets.ktcore/presentation/src/commonMain/composeResources/values/strings.xmlfeature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/import/ExternalImportViewModel.ktfeature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsViewModel.ktfeature/home/presentation/src/commonMain/kotlin/zed/rainxch/home/presentation/HomeConsentGateViewModel.ktfeature/home/presentation/src/commonMain/kotlin/zed/rainxch/home/presentation/HomeRoot.ktroadmap/E6_CLIENT_HANDOFF.md
✅ Files skipped from review due to trivial changes (2)
- core/presentation/src/commonMain/composeResources/values/strings.xml
- roadmap/E6_CLIENT_HANDOFF.md
🚧 Files skipped from review as they are similar to previous changes (1)
- core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/UpdateCheckWorker.kt
Comment on lines
+333
to
+352
| private inline fun <T> safeCall(block: () -> Result<T>): Result<T> { | ||
| val result = | ||
| try { | ||
| block() | ||
| } catch (e: CancellationException) { | ||
| throw e | ||
| } catch (e: Exception) { | ||
| Result.failure(e) | ||
| } | ||
| firePerProxyOutcome(result.isSuccess) | ||
| return result | ||
| } | ||
|
|
||
| private fun firePerProxyOutcome(success: Boolean) { | ||
| val proxy = currentProxyConfig | ||
| if (proxy is ProxyConfig.None || proxy is ProxyConfig.System) return | ||
| productTelemetry.fire( | ||
| name = ProductTelemetryEvents.PROXY_USED, | ||
| props = mapOf(ProductTelemetryProps.SUCCESS to success.toString()), | ||
| ) |
Contributor
There was a problem hiding this comment.
Snapshot the proxy config before the request starts.
safeCall() emits PROXY_USED using the live currentProxyConfig after block() returns. If the user changes proxy settings while the request is in flight, the event gets attributed to the new config, or dropped entirely when it flips to None/System, even though the old client handled the call.
♻️ Suggested fix
- private inline fun <T> safeCall(block: () -> Result<T>): Result<T> {
+ private inline fun <T> safeCall(block: () -> Result<T>): Result<T> {
+ val proxyAtStart = currentProxyConfig
val result =
try {
block()
} catch (e: CancellationException) {
throw e
} catch (e: Exception) {
Result.failure(e)
}
- firePerProxyOutcome(result.isSuccess)
+ firePerProxyOutcome(proxyAtStart, result.isSuccess)
return result
}
- private fun firePerProxyOutcome(success: Boolean) {
- val proxy = currentProxyConfig
+ private fun firePerProxyOutcome(proxy: ProxyConfig, success: Boolean) {
if (proxy is ProxyConfig.None || proxy is ProxyConfig.System) return
productTelemetry.fire(
name = ProductTelemetryEvents.PROXY_USED,
props = mapOf(ProductTelemetryProps.SUCCESS to success.toString()),
)📝 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.
Suggested change
| private inline fun <T> safeCall(block: () -> Result<T>): Result<T> { | |
| val result = | |
| try { | |
| block() | |
| } catch (e: CancellationException) { | |
| throw e | |
| } catch (e: Exception) { | |
| Result.failure(e) | |
| } | |
| firePerProxyOutcome(result.isSuccess) | |
| return result | |
| } | |
| private fun firePerProxyOutcome(success: Boolean) { | |
| val proxy = currentProxyConfig | |
| if (proxy is ProxyConfig.None || proxy is ProxyConfig.System) return | |
| productTelemetry.fire( | |
| name = ProductTelemetryEvents.PROXY_USED, | |
| props = mapOf(ProductTelemetryProps.SUCCESS to success.toString()), | |
| ) | |
| private inline fun <T> safeCall(block: () -> Result<T>): Result<T> { | |
| val proxyAtStart = currentProxyConfig | |
| val result = | |
| try { | |
| block() | |
| } catch (e: CancellationException) { | |
| throw e | |
| } catch (e: Exception) { | |
| Result.failure(e) | |
| } | |
| firePerProxyOutcome(proxyAtStart, result.isSuccess) | |
| return result | |
| } | |
| private fun firePerProxyOutcome(proxy: ProxyConfig, success: Boolean) { | |
| if (proxy is ProxyConfig.None || proxy is ProxyConfig.System) return | |
| productTelemetry.fire( | |
| name = ProductTelemetryEvents.PROXY_USED, | |
| props = mapOf(ProductTelemetryProps.SUCCESS to success.toString()), | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@core/data/src/commonMain/kotlin/zed/rainxch/core/data/network/BackendApiClient.kt`
around lines 333 - 352, safeCall currently reads currentProxyConfig after
block() returns, which can misattribute or drop PROXY_USED events; snapshot the
proxy configuration before invoking block() and use that snapshot when firing
telemetry. Concretely, in safeCall() capture val proxySnapshot =
currentProxyConfig before calling block(), then change firePerProxyOutcome to
accept a proxy parameter (or add a private overload) and call
firePerProxyOutcome(proxySnapshot, result.isSuccess) so the telemetry uses the
proxySnapshot and not the live currentProxyConfig; reference safeCall,
firePerProxyOutcome, currentProxyConfig, ProxyConfig.None/ProxyConfig.System,
productTelemetry and ProductTelemetryEvents.PROXY_USED.
Comment on lines
+144
to
+146
| private suspend fun consent(): ProductTelemetryConsent = | ||
| runCatching { tweaksRepository.getProductTelemetryConsent().first() } | ||
| .getOrDefault(ProductTelemetryConsent.NotYetAsked) |
Contributor
There was a problem hiding this comment.
Re-throw CancellationException in consent() helper.
Similar to the fix applied in the init block, runCatching here swallows CancellationException, which can delay coroutine cancellation.
🛡️ Proposed fix
private suspend fun consent(): ProductTelemetryConsent =
- runCatching { tweaksRepository.getProductTelemetryConsent().first() }
- .getOrDefault(ProductTelemetryConsent.NotYetAsked)
+ try {
+ tweaksRepository.getProductTelemetryConsent().first()
+ } catch (e: CancellationException) {
+ throw e
+ } catch (_: Exception) {
+ ProductTelemetryConsent.NotYetAsked
+ }📝 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.
Suggested change
| private suspend fun consent(): ProductTelemetryConsent = | |
| runCatching { tweaksRepository.getProductTelemetryConsent().first() } | |
| .getOrDefault(ProductTelemetryConsent.NotYetAsked) | |
| private suspend fun consent(): ProductTelemetryConsent = | |
| try { | |
| tweaksRepository.getProductTelemetryConsent().first() | |
| } catch (e: CancellationException) { | |
| throw e | |
| } catch (_: Exception) { | |
| ProductTelemetryConsent.NotYetAsked | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@core/data/src/commonMain/kotlin/zed/rainxch/core/data/telemetry/ProductTelemetryImpl.kt`
around lines 144 - 146, The helper consent() in ProductTelemetryImpl currently
uses runCatching which swallows CancellationException and delays coroutine
cancellation; update consent() (the function calling
tweaksRepository.getProductTelemetryConsent().first()) to rethrow
CancellationException instead of swallowing it — e.g. use a try/catch around the
call or check the thrown exception from runCatching and if it is a
CancellationException rethrow it, otherwise return
ProductTelemetryConsent.NotYetAsked as the default.
Comment on lines
+138
to
+147
| TrackFirstPaint(isReady = !state.isLoading) { ms -> | ||
| productTelemetry.fire( | ||
| name = ProductTelemetryEvents.FIRST_PAINT_MS, | ||
| props = | ||
| mapOf( | ||
| ProductTelemetryProps.SCREEN to "home", | ||
| ProductTelemetryProps.BUCKET to TelemetryBuckets.durationMs(ms), | ||
| ), | ||
| ) | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n--- HomeState ---\n'
fd 'HomeState.kt$' -a | while read -r f; do
echo "FILE: $f"
sed -n '1,220p' "$f"
done
printf '\n--- HomeViewModel ---\n'
fd 'HomeViewModel.kt$' -a | while read -r f; do
echo "FILE: $f"
sed -n '1,260p' "$f"
doneRepository: OpenHub-Store/GitHub-Store
Length of output: 12068
Confirm that HomeState() initializes with isLoading = false, causing near-zero paint telemetry.
The initial state defaults to isLoading = false, and because .stateIn(initialValue = HomeState()) emits immediately before the onStart block executes loadRepos(isInitial = true), TrackFirstPaint will fire on the first emission with !state.isLoading true—before any content actually loads. Since TrackFirstPaint fires only once, this records the paint time near zero and never self-corrects.
Either initialize HomeState with isLoading = true, or gate on a stronger readiness signal (e.g., repos are loaded and not empty).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@feature/home/presentation/src/commonMain/kotlin/zed/rainxch/home/presentation/HomeRoot.kt`
around lines 138 - 147, HomeState currently defaults isLoading=false so the
first .stateIn(initialValue = HomeState()) emission makes
TrackFirstPaint(isReady = !state.isLoading) fire immediately and record
near-zero paint; fix by either initializing HomeState with isLoading = true or
change the readiness predicate passed to TrackFirstPaint to a stronger signal
(for example, check that repos are loaded and non-empty or a dedicated isReady
flag set after loadRepos(isInitial = true) completes) — update the HomeState
constructor or the TrackFirstPaint call accordingly and ensure
loadRepos(isInitial = true) clears that loading flag when done.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
feature/home/presentation/src/commonMain/kotlin/zed/rainxch/home/presentation/components/PrivacyCollectedView.kt (1)
76-148: Avoid hand-maintaining a second telemetry schema list in the UI.This screen duplicates the event contract as raw strings, separate from
ProductTelemetryEventsand the audit allowlist. The next rename/addition can leave the consent copy stale without any compiler help. Prefer sourcing these names from shared telemetry metadata, or at least from the existing telemetry constants.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/home/presentation/src/commonMain/kotlin/zed/rainxch/home/presentation/components/PrivacyCollectedView.kt` around lines 76 - 148, The UI currently hand-maintains telemetry event names in the sections list (CollectedSection/CollectedEvent) and neverCollected constants, duplicating the canonical ProductTelemetryEvents/audit allowlist and risking drift; replace the hard-coded string event IDs with references to the shared telemetry metadata/constants (e.g., use ProductTelemetryEvents.<EVENT>.id or the shared audit allowlist entries) when constructing the sections and neverCollected lists so the UI derives labels/IDs from the single source of truth instead of raw strings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@feature/home/presentation/src/commonMain/kotlin/zed/rainxch/home/presentation/components/PrivacyCollectedView.kt`:
- Around line 76-148: The UI currently hand-maintains telemetry event names in
the sections list (CollectedSection/CollectedEvent) and neverCollected
constants, duplicating the canonical ProductTelemetryEvents/audit allowlist and
risking drift; replace the hard-coded string event IDs with references to the
shared telemetry metadata/constants (e.g., use ProductTelemetryEvents.<EVENT>.id
or the shared audit allowlist entries) when constructing the sections and
neverCollected lists so the UI derives labels/IDs from the single source of
truth instead of raw strings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: da5be5d2-1a26-4a03-a3ae-bf69ad77806e
📒 Files selected for processing (4)
core/presentation/src/commonMain/composeResources/values/strings.xmlfeature/home/presentation/src/commonMain/kotlin/zed/rainxch/home/presentation/HomeRoot.ktfeature/home/presentation/src/commonMain/kotlin/zed/rainxch/home/presentation/components/PrivacyCollectedView.ktfeature/home/presentation/src/commonMain/kotlin/zed/rainxch/home/presentation/components/ProductTelemetryConsentSheet.kt
✅ Files skipped from review due to trivial changes (1)
- core/presentation/src/commonMain/composeResources/values/strings.xml
🚧 Files skipped from review as they are similar to previous changes (1)
- feature/home/presentation/src/commonMain/kotlin/zed/rainxch/home/presentation/components/ProductTelemetryConsentSheet.kt
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
feature/home/presentation/src/commonMain/kotlin/zed/rainxch/home/presentation/components/PrivacyCollectedView.kt (1)
82-145: Consider centralizing event-name literals to a shared source.Using raw string literals for telemetry event names here is easy to drift over time. A shared constant source (same one used by telemetry emission/audit) would reduce typo/mismatch risk in privacy copy.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/home/presentation/src/commonMain/kotlin/zed/rainxch/home/presentation/components/PrivacyCollectedView.kt` around lines 82 - 145, The privacy view currently embeds raw telemetry event-name strings inside the sections list (see CollectedEvent instances in the sections val), which can drift from the telemetry emission source; replace those string literals with constants from a shared telemetry/events definitions module (the same constants used when emitting/auditing events), e.g., import and use EVENT_APP_LAUNCHED, EVENT_SESSION_DURATION, etc., for all CollectedEvent(...) entries in the sections list so the privacy copy references the canonical names used by telemetry.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@composeApp/src/jvmMain/kotlin/zed/rainxch/githubstore/DesktopApp.kt`:
- Around line 121-129: The shutdown path uses ProductTelemetry.fire() which
enqueues asynchronously and can drop termination-sensitive events (e.g.,
SESSION_DURATION and CRASH); modify the shutdown and crash handlers (the
onCloseRequest block and the crash handler locations referenced) to call a
synchronous/suspending enqueue method on ProductTelemetry (add or use a suspend
enqueue/enqueueBlocking on ProductTelemetryImpl) instead of fire(), await that
enqueue completion, then call the existing flush/awaitFlush routine before
calling exitApplication() or performing the final crash flush; update
ProductTelemetryImpl.fire() users to use the new suspend enqueue for
termination-sensitive events and keep async fire() for non-critical telemetry.
---
Nitpick comments:
In
`@feature/home/presentation/src/commonMain/kotlin/zed/rainxch/home/presentation/components/PrivacyCollectedView.kt`:
- Around line 82-145: The privacy view currently embeds raw telemetry event-name
strings inside the sections list (see CollectedEvent instances in the sections
val), which can drift from the telemetry emission source; replace those string
literals with constants from a shared telemetry/events definitions module (the
same constants used when emitting/auditing events), e.g., import and use
EVENT_APP_LAUNCHED, EVENT_SESSION_DURATION, etc., for all CollectedEvent(...)
entries in the sections list so the privacy copy references the canonical names
used by telemetry.
🪄 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: 2bf5d74f-a3bf-4e86-b082-791db9b4f828
📒 Files selected for processing (4)
composeApp/src/androidMain/kotlin/zed/rainxch/githubstore/app/GithubStoreApp.ktcomposeApp/src/jvmMain/kotlin/zed/rainxch/githubstore/DesktopApp.ktcore/presentation/src/commonMain/composeResources/values/strings.xmlfeature/home/presentation/src/commonMain/kotlin/zed/rainxch/home/presentation/components/PrivacyCollectedView.kt
✅ Files skipped from review due to trivial changes (1)
- core/presentation/src/commonMain/composeResources/values/strings.xml
Comment on lines
+72
to
+86
| Lifecycle.Event.ON_STOP -> { | ||
| val seconds = foregroundStart?.elapsedNow()?.inWholeSeconds | ||
| foregroundStart = null | ||
| val telemetry = get<ProductTelemetry>() | ||
| if (seconds != null) { | ||
| telemetry.fire( | ||
| name = ProductTelemetryEvents.SESSION_DURATION, | ||
| props = mapOf(ProductTelemetryProps.SECONDS to seconds.toString()), | ||
| ) | ||
| } | ||
| // Off-main bounded flush — ON_STOP runs on the main | ||
| // thread, never block it on the network. | ||
| appScope.launch { | ||
| withTimeoutOrNull(2_000) { telemetry.flush() } | ||
| } |
Contributor
There was a problem hiding this comment.
fire() races the flush in both lifecycle-sensitive paths.
ProductTelemetryImpl.fire() appends asynchronously, while both of these paths flush right away. If the enqueue hasn't run yet, SESSION_DURATION/CRASH is dropped even though the flush completes normally. This needs a synchronous/suspend enqueue path for termination-sensitive events before flushing.
Also applies to: 99-110
Comment on lines
+121
to
+129
| onCloseRequest = { | ||
| val telemetry = GlobalContext.get().get<ProductTelemetry>() | ||
| ColdStart.elapsedSeconds()?.let { seconds -> | ||
| telemetry.fire( | ||
| name = ProductTelemetryEvents.SESSION_DURATION, | ||
| props = mapOf(ProductTelemetryProps.SECONDS to seconds.toString()), | ||
| ) | ||
| } | ||
| exitApplication() |
Contributor
There was a problem hiding this comment.
fire() can lose shutdown-path events here.
ProductTelemetryImpl.fire() only buffers after hopping into its own coroutine. In both the close path and the crash path, this code fires an event and then immediately exits or flushes. That means SESSION_DURATION/CRASH can miss the buffer entirely even when the flush succeeds. Please use a synchronous/suspend enqueue path for termination-sensitive events, then flush after that enqueue completes.
Also applies to: 151-162, 165-178
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@composeApp/src/jvmMain/kotlin/zed/rainxch/githubstore/DesktopApp.kt` around
lines 121 - 129, The shutdown path uses ProductTelemetry.fire() which enqueues
asynchronously and can drop termination-sensitive events (e.g., SESSION_DURATION
and CRASH); modify the shutdown and crash handlers (the onCloseRequest block and
the crash handler locations referenced) to call a synchronous/suspending enqueue
method on ProductTelemetry (add or use a suspend enqueue/enqueueBlocking on
ProductTelemetryImpl) instead of fire(), await that enqueue completion, then
call the existing flush/awaitFlush routine before calling exitApplication() or
performing the final crash flush; update ProductTelemetryImpl.fire() users to
use the new suspend enqueue for termination-sensitive events and keep async
fire() for non-critical telemetry.
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
POST /v1/telemetry/eventsbackend.TelemetryRepositorytoProductTelemetryatomically (no double-emission); deletes the five out-of-scope legacy import calls listed in the handoff §3.4.PrivacyAuditTestpinning the client allowlist to the backend's locked schema; adds bounded-flush hooks on AndroidProcessLifecycleOwnerON_STOP and JVMRuntime.addShutdownHook.fromnav arg intoDetailsScreenso DETAILS_VIEWED can carry a categorical "search" / "category" / "library" / "link" prop without leaking repo identity.Test plan
./gradlew :composeApp:compileKotlinJvm :composeApp:compileDebugKotlinAndroid— both targets compile clean../gradlew :core:domain:jvmTest—PrivacyAuditTestpasses (allowlist matches backend, no PII keys, all prop keys snake_case ≤32 chars).telemetry_eventson the VPS, expect rows forapp_launched,cold_start_ms,first_paint_ms, plus whatever other events were exercised. Toggle off in Settings → use → re-query → no new rows.Summary by CodeRabbit
New Features
Chores
Documentation
Tests
Behavioral Change