Repository navigation
feat: add privacy-safe usage telemetry - #173
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughAdds a catalog-driven, sanitized telemetry pipeline: backend event catalog and sanitizer, in-process emitter and SSE forwarding, POST endpoint, frontend telemetry API and GA forwarding with a localStorage queue, instrumentation across onboarding/workflows/projects/artifacts/chat/files, UI toggle defaulting to enabled, types, docs, and tests. ChangesAnonymous Telemetry End-to-End
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Runtime as runtimeApi
participant Server as /api/telemetry/event
participant Track as trackTelemetry()
participant Emitter as telemetryEmitter
App->>Runtime: runtimeApi.emit(event, payload)
Runtime->>Server: POST /api/telemetry/event
Server->>Track: trackTelemetry(name,payload,settings)
Track->>Track: sanitizeEvent()
Track->>Emitter: emit('event', {name,payload})
Emitter->>App: SSE 'telemetry-event' (subscribers)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
🚥 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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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: 7
🧹 Nitpick comments (3)
src/pages/GlobalSettings.tsx (1)
170-172: ⚡ Quick winPreserve existing telemetry subfields when applying defaults.
This merge replaces
loadedSettings.telemetrywith{ enabled }, which can silently discard other telemetry properties on next save. Merge the existing object first.Suggested diff
- telemetry: { - enabled: loadedSettings?.telemetry?.enabled ?? true, - }, + telemetry: { + ...(loadedSettings?.telemetry || {}), + enabled: loadedSettings?.telemetry?.enabled ?? true, + },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/GlobalSettings.tsx` around lines 170 - 172, The code currently replaces loadedSettings.telemetry with a new object containing only enabled, which drops other telemetry subfields; change the assignment in GlobalSettings.tsx to merge the existing telemetry object first (spread loadedSettings?.telemetry or an empty object) and then set enabled using the nullish fallback (enabled: loadedSettings?.telemetry?.enabled ?? true) so existing properties are preserved while applying the default.src/api/runtime.ts (2)
33-33: ⚡ Quick winUse
@/alias for internal API imports in this file.This changed import uses a relative path instead of the repo-standard alias.
As per coding guidelines: `Use `@/` path alias for all internal module imports in React/TypeScript frontend code (maps to ./src/)`.♻️ Proposed fix
-import { SERVER_URL, serverOnline, serverFetch, systemApi, secretsApi, settingsApi, chatApi, authApi, projectsApi, projectsApiExtended, filesApi, artifactsApi, workflowsApi, skillsApi, mcpApi, researchLogApi, telemetryApi } from './server'; +import { SERVER_URL, serverOnline, serverFetch, systemApi, secretsApi, settingsApi, chatApi, authApi, projectsApi, projectsApiExtended, filesApi, artifactsApi, workflowsApi, skillsApi, mcpApi, researchLogApi, telemetryApi } from '`@/api/server`';🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/runtime.ts` at line 33, The import in runtime.ts uses a relative path './server' instead of the repo-standard alias; replace the relative import with the '`@/`...' alias (e.g., import { SERVER_URL, serverOnline, serverFetch, systemApi, secretsApi, settingsApi, chatApi, authApi, projectsApi, projectsApiExtended, filesApi, artifactsApi, workflowsApi, skillsApi, mcpApi, researchLogApi, telemetryApi } from '`@/api/server`') so all internal imports in this file follow the alias convention and map to ./src/.
484-486: ⚡ Quick winMake
emitfire-and-forget to avoid accidental await latency.Awaiting telemetry here can block callers on network/health-check delays.
emitshould stay best-effort and non-blocking.♻️ Proposed fix
async emit(event: string, payload?: any): Promise<void> { - await telemetryApi.track(event, payload || {}); + void telemetryApi.track(event, payload ?? {}); },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/runtime.ts` around lines 484 - 486, The emit method currently awaits telemetryApi.track, blocking callers; change emit to fire-and-forget by invoking telemetryApi.track(event, payload || {}) without awaiting it and attach a .catch(...) to swallow/log errors to avoid unhandled promise rejections. Locate the async emit(event: string, payload?: any): Promise<void> method and replace the awaited call with a non-awaited call to telemetryApi.track(...) and add minimal error handling (e.g., .catch(() => {}) or log via the existing logger) so telemetry remains best-effort and non-blocking.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@node-backend/lib/orchestrator.mjs`:
- Around line 216-218: The telemetry call inside the artifact loop
(trackTelemetry used in runAgentLoop over artifactChanges) must be best-effort:
wrap the await trackTelemetry(...) call in a try/catch so any thrown error is
caught and does not propagate and fail runAgentLoop; on catch, log the error
(using the existing logger in scope, e.g., processLogger or console) with
context including the artifactType/change, but do not rethrow so artifact
processing remains successful.
In `@node-backend/lib/telemetry/index.mjs`:
- Around line 156-175: The flush logic (flushPromise) snapshots the queue with
readQueue() and later calls writeQueue(remaining), so events appended between
the initial read and the write can be lost; modify the flush flow to merge any
new events that arrived after the snapshot before writing the remaining items
back (e.g., after a successful POST, call readQueue() again and
writeQueue(concat(remaining, freshlyRead)) or otherwise perform a
compare-and-swap style update) and apply the same fix to the other queue-flush
area referenced (lines ~89-112); reference functions: flushPromise, readQueue,
writeQueue, and the batch/remaining logic to locate and update the code.
- Around line 164-171: The fetch call used to POST telemetry (the
fetch(endpoint, { method: 'POST', ... body: JSON.stringify({ events: batch })
})) has no timeout and can hang flush cycles (flushPromise); add an
AbortController with a setTimeout to cancel the request after a sensible timeout
(e.g. 5s), pass controller.signal into fetch, clear the timer on success, and
ensure the surrounding logic that resolves/rejects flushPromise correctly
handles an aborted/timeout error by treating it as a failed flush (log or
swallow as appropriate) so flushPromise cannot remain blocked indefinitely.
In `@node-backend/lib/workflows.mjs`:
- Around line 228-229: The awaited telemetry calls (trackTelemetry) must not be
allowed to throw and break workflow execution: wrap each await
trackTelemetry(...) in a try/catch that catches any error and logs it (or
silently handles it) without rethrowing so the workflow continues; apply this
pattern around the trackTelemetry call before invoking
orchestrator.runAgentLoop(...) and for the other trackTelemetry usages
referenced in this file (the similar calls near where orchestrator.runAgentLoop
and other agent step/finalization logic are executed) so telemetry failures
never affect workflow correctness.
- Around line 238-241: The telemetry call for "agent.run.completed" currently
treats any model_used value other than 'error' as success, which incorrectly
counts 'aborted' as successful; update the success logic used in the
trackTelemetry call (symbol: trackTelemetry, event: 'agent.run.completed',
property: result?.metadata?.model_used) so that 'aborted' is treated as a
failure (e.g., require model_used === 'success' or explicitly exclude both
'error' and 'aborted') and ensure undefined/null cases are handled consistently.
- Around line 253-257: The telemetry call for event 'agent.run.failed' currently
sends durationMs: 0; update it to send the actual elapsed time by computing
durationMs from the run's start timestamp or the error metadata: use a recorded
start time (e.g., runStartTime or workflowStartTimestamp) or the stepError
duration field (e.g., stepError.durationMs || stepError.duration) to compute
Date.now() - runStartTime, and pass that value to trackTelemetry instead of 0,
with a safe fallback to 0 if no timing data exists; keep the call to
trackTelemetry('agent.run.failed', {..., errorCode:
telemetryErrorCode(stepError)}, settings) intact.
In `@node-backend/server.mjs`:
- Line 1210: The startup call using readGlobalSettings().then(... ) can yield an
unhandled rejection if readGlobalSettings() fails; append a .catch(...) to that
promise chain to swallow or log the error and keep startup resilient (e.g., void
readGlobalSettings().then(settings => trackTelemetry('app.launched', {},
settings)).catch(err => { processLogger?.warn('Failed to read global settings
for telemetry', err) || console.warn('Failed to read global settings for
telemetry', err); })), referencing the existing readGlobalSettings and
trackTelemetry calls so the error is handled explicitly.
---
Nitpick comments:
In `@src/api/runtime.ts`:
- Line 33: The import in runtime.ts uses a relative path './server' instead of
the repo-standard alias; replace the relative import with the '`@/`...' alias
(e.g., import { SERVER_URL, serverOnline, serverFetch, systemApi, secretsApi,
settingsApi, chatApi, authApi, projectsApi, projectsApiExtended, filesApi,
artifactsApi, workflowsApi, skillsApi, mcpApi, researchLogApi, telemetryApi }
from '`@/api/server`') so all internal imports in this file follow the alias
convention and map to ./src/.
- Around line 484-486: The emit method currently awaits telemetryApi.track,
blocking callers; change emit to fire-and-forget by invoking
telemetryApi.track(event, payload || {}) without awaiting it and attach a
.catch(...) to swallow/log errors to avoid unhandled promise rejections. Locate
the async emit(event: string, payload?: any): Promise<void> method and replace
the awaited call with a non-awaited call to telemetryApi.track(...) and add
minimal error handling (e.g., .catch(() => {}) or log via the existing logger)
so telemetry remains best-effort and non-blocking.
In `@src/pages/GlobalSettings.tsx`:
- Around line 170-172: The code currently replaces loadedSettings.telemetry with
a new object containing only enabled, which drops other telemetry subfields;
change the assignment in GlobalSettings.tsx to merge the existing telemetry
object first (spread loadedSettings?.telemetry or an empty object) and then set
enabled using the nullish fallback (enabled: loadedSettings?.telemetry?.enabled
?? true) so existing properties are preserved while applying the default.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: fa632d4b-e27c-47c6-9dc6-bda19108a5b7
📒 Files selected for processing (16)
TELEMETRY.mdnode-backend/lib/orchestrator.mjsnode-backend/lib/telemetry/catalog.mjsnode-backend/lib/telemetry/index.mjsnode-backend/lib/workflows.mjsnode-backend/server.mjssrc/api/app.tssrc/api/contracts.tssrc/api/runtime.tssrc/api/server.tssrc/api/types.tssrc/components/Installation/InstallationWizard.tsxsrc/components/settings/SystemSettings.tsxsrc/pages/GlobalSettings.tsxsrc/pages/Onboarding.tsxtests/telemetry.test.mjs
AssafMiron
requested changes
May 21, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/App.tsx (1)
144-144: ⚡ Quick winConsider making the Google Analytics measurement ID configurable.
The GA measurement ID
'G-4K6J4VT5DR'is hardcoded on Line 144 (and duplicated on Line 153). For production flexibility and testing, consider moving this to an environment variable or configuration file.♻️ Suggested refactor to use environment variable
- ReactGA.initialize('G-4K6J4VT5DR'); + const gaId = import.meta.env.VITE_GA_MEASUREMENT_ID || 'G-4K6J4VT5DR'; + ReactGA.initialize(gaId); gaInitialized = true;And similarly for Line 153.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/App.tsx` at line 144, The GA measurement ID is hardcoded in ReactGA.initialize calls in App.tsx (both occurrences); change these to read from a configurable environment variable (e.g. process.env.REACT_APP_GA_MEASUREMENT_ID or your app's config) and use that variable in both ReactGA.initialize calls instead of the literal 'G-4K6J4VT5DR'; also add a defensive check in the App component (or initAnalytics function) to only call ReactGA.initialize when the environment variable is present and log or skip initialization if it's missing so tests and non-production builds can opt out.node-backend/server.mjs (2)
556-593: ⚡ Quick win
provider.detectedtelemetry can be lost when authentication probes throw.For Claude/Gemini/OpenAI detection,
provider.checkAuthentication()is called betweenstartedand thetrack()call. If that probe throws (network, CLI error, missing config), the route returns 500 via the outer catch and thetrack('provider.detected', …)line is never reached — even though the CLI detection itself already produced a usablestatus.Since
successis derived fromstatus.installed(not from authentication), consider either tracking before the auth probe or wrapping it so the telemetry fires regardless.♻️ Example refactor (claude branch — same pattern applies to gemini/openai)
if (req.method === 'GET' && url.pathname === '/api/system/detect/claude') { const started = Date.now(); const settings = await readGlobalSettings(); const status = await checkCli('claude'); let authenticated = false; - if (status.installed) { - const provider = await AIService.createProvider('claudeCode', settings); - authenticated = await provider.checkAuthentication(); - } - track('provider.detected', { provider: 'claudeCode', success: !!status.installed, durationMs: Date.now() - started }, settings); + try { + if (status.installed) { + const provider = await AIService.createProvider('claudeCode', settings); + authenticated = await provider.checkAuthentication(); + } + } finally { + track('provider.detected', { provider: 'claudeCode', success: !!status.installed, durationMs: Date.now() - started }, settings); + } return sendJson(res, 200, { ...status, authenticated }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@node-backend/server.mjs` around lines 556 - 593, The telemetry call track('provider.detected', ...) can be skipped if provider.checkAuthentication() throws; to fix, ensure track is always called regardless of auth probe by either moving the track(...) invocation to immediately after obtaining status (after checkCli/resolveCliCommand) or by wrapping the authentication probe in a try/catch: call AIService.createProvider(...) and run provider.checkAuthentication() inside a try block that falls back to authenticated = false on error (and optionally log), then always call track('provider.detected', { provider: '<name>', success: !!status.installed, durationMs: Date.now() - started }, settings) before returning sendJson(...); apply this change to the Claude (checkCli('claude') / 'claudeCode'), Gemini (checkCli('gemini') / 'geminiCli') and OpenAI (resolveCliCommand('codex','openai') / 'openAiCli') branches so telemetry cannot be lost.
487-492: 💤 Low valueConsider basic protection for the public
/api/telemetry/eventendpoint.This endpoint accepts arbitrary client POSTs and writes to
queue.ndjsonfor every accepted event.sanitizeEventintrackTelemetryrejects unknown event names and strips unknown keys, so PII leakage is well-guarded. However, there is no rate limit and no body-size cap visible here, so a misbehaving page on a hostile origin (CORS is wildcard-open for SSE) could still flood the local queue or wedge disk I/O with rapid POSTs.At minimum, consider a lightweight per-process rate limit on this route and/or asserting Content-Length before reading the body.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@node-backend/server.mjs` around lines 487 - 492, The telemetry POST handler for '/api/telemetry/event' currently reads the entire body and forwards events to trackTelemetry (which writes queue.ndjson) without any size or rate protections; add a lightweight per-process rate limiter and a Content-Length/body-size guard before calling readJson to prevent flooding. Concretely, in the POST handler (the block using req.method, url.pathname, readJson, trackTelemetry, sendError, sendNoContent, and readGlobalSettings) check req.headers['content-length'] and reject (400) if missing or exceeds a reasonable cap (e.g., a few KB), and implement a simple in-memory token-bucket or fixed-window counter keyed to a single global bucket that rejects requests (429) when the rate (e.g., requests per second/minute) is exceeded; only call readJson and trackTelemetry after these checks pass. Ensure error responses use sendError/sendNoContent as currently used.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@TELEMETRY.md`:
- Around line 69-72: Update TELEMETRY.md to explicitly clarify the default
behavior of PRODUCTOS_TELEMETRY_ENDPOINT and its relationship to GA4: state that
PRODUCTOS_TELEMETRY_ENDPOINT default is unset (no network sends, events queued
locally and retried) and that GA4 client-side telemetry (the GA4 integration
described earlier) is independent and still sends from the client to Google
Analytics unless client telemetry is disabled; also explain that the backend
telemetry endpoint is for server-side/internal telemetry ingestion (different
from client-facing GA4), and mention whether there is any default endpoint value
or placeholder (e.g., none/unset) so readers know to configure
PRODUCTOS_TELEMETRY_ENDPOINT when they want server-side forwarding.
---
Nitpick comments:
In `@node-backend/server.mjs`:
- Around line 556-593: The telemetry call track('provider.detected', ...) can be
skipped if provider.checkAuthentication() throws; to fix, ensure track is always
called regardless of auth probe by either moving the track(...) invocation to
immediately after obtaining status (after checkCli/resolveCliCommand) or by
wrapping the authentication probe in a try/catch: call
AIService.createProvider(...) and run provider.checkAuthentication() inside a
try block that falls back to authenticated = false on error (and optionally
log), then always call track('provider.detected', { provider: '<name>', success:
!!status.installed, durationMs: Date.now() - started }, settings) before
returning sendJson(...); apply this change to the Claude (checkCli('claude') /
'claudeCode'), Gemini (checkCli('gemini') / 'geminiCli') and OpenAI
(resolveCliCommand('codex','openai') / 'openAiCli') branches so telemetry cannot
be lost.
- Around line 487-492: The telemetry POST handler for '/api/telemetry/event'
currently reads the entire body and forwards events to trackTelemetry (which
writes queue.ndjson) without any size or rate protections; add a lightweight
per-process rate limiter and a Content-Length/body-size guard before calling
readJson to prevent flooding. Concretely, in the POST handler (the block using
req.method, url.pathname, readJson, trackTelemetry, sendError, sendNoContent,
and readGlobalSettings) check req.headers['content-length'] and reject (400) if
missing or exceeds a reasonable cap (e.g., a few KB), and implement a simple
in-memory token-bucket or fixed-window counter keyed to a single global bucket
that rejects requests (429) when the rate (e.g., requests per second/minute) is
exceeded; only call readJson and trackTelemetry after these checks pass. Ensure
error responses use sendError/sendNoContent as currently used.
In `@src/App.tsx`:
- Line 144: The GA measurement ID is hardcoded in ReactGA.initialize calls in
App.tsx (both occurrences); change these to read from a configurable environment
variable (e.g. process.env.REACT_APP_GA_MEASUREMENT_ID or your app's config) and
use that variable in both ReactGA.initialize calls instead of the literal
'G-4K6J4VT5DR'; also add a defensive check in the App component (or
initAnalytics function) to only call ReactGA.initialize when the environment
variable is present and log or skip initialization if it's missing so tests and
non-production builds can opt out.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4e870214-7c45-42f7-82a8-8f3d7982c5a4
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (11)
TELEMETRY.mdnode-backend/lib/orchestrator.mjsnode-backend/lib/telemetry/catalog.mjsnode-backend/lib/telemetry/index.mjsnode-backend/lib/workflows.mjsnode-backend/server.mjspackage.jsonsrc/App.tsxsrc/api/runtime.tssrc/components/settings/SystemSettings.tsxsrc/pages/GlobalSettings.tsx
Comment on lines
+69
to
+72
| POST $PRODUCTOS_TELEMETRY_ENDPOINT | ||
| ``` | ||
|
|
||
| If `PRODUCTOS_TELEMETRY_ENDPOINT` is not configured, ProductOS queues events locally but does not send network requests. Failed sends stay on disk and are retried later. |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Clarify the default behavior for PRODUCTOS_TELEMETRY_ENDPOINT.
The documentation states that if PRODUCTOS_TELEMETRY_ENDPOINT is not configured, events are queued locally but never sent. This is privacy-friendly but may not align with the GA4 integration documented earlier (Lines 21-26), which suggests client-side telemetry is sent to Google Analytics regardless of backend endpoint configuration.
Consider clarifying:
- Is GA4 client-side telemetry independent of the backend endpoint?
- Does the backend endpoint serve a different purpose (e.g., internal analytics)?
- Should the docs mention the default endpoint value, if any?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@TELEMETRY.md` around lines 69 - 72, Update TELEMETRY.md to explicitly clarify
the default behavior of PRODUCTOS_TELEMETRY_ENDPOINT and its relationship to
GA4: state that PRODUCTOS_TELEMETRY_ENDPOINT default is unset (no network sends,
events queued locally and retried) and that GA4 client-side telemetry (the GA4
integration described earlier) is independent and still sends from the client to
Google Analytics unless client telemetry is disabled; also explain that the
backend telemetry endpoint is for server-side/internal telemetry ingestion
(different from client-facing GA4), and mention whether there is any default
endpoint value or placeholder (e.g., none/unset) so readers know to configure
PRODUCTOS_TELEMETRY_ENDPOINT when they want server-side forwarding.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
TELEMETRY.md (1)
7-12:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDoc claims about
installIdand env metadata don't match the current implementation.The simplified
node-backend/lib/telemetry/index.mjsno longer persists anything to disk (notelemetry/telemetry.json, noinstallIdgeneration) andsanitizeEventdoes not augment payloads with version, runtime, Node version, OS, or arch — it strips everything except the per-event allowlisted keys inTELEMETRY_EVENTS. Either reinstate the metadata in the emit path, or update the doc to reflect that events carry only the event name + per-event allowlisted payload.Note that GA4 will automatically capture client tech metadata (browser/OS/country) — that part is already covered in the "Google Analytics Integration" section, but it's distinct from a backend-generated
installIdor Node runtime info.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@TELEMETRY.md` around lines 7 - 12, The docs claim telemetry includes a persisted installId and backend-added env metadata, but the current implementation in node-backend/lib/telemetry/index.mjs (notably sanitizeEvent and TELEMETRY_EVENTS) strips all non-allowlisted keys and does not persist installId; update either the code or the docs. To fix: either (A) reinstate metadata by generating/persisting an installId and augmenting events in the emit path before sanitizeEvent (identify the emit/send function in index.mjs and add installId, ProductOS/version, runtime/node version, OS, arch) and ensure persistence to telemetry/telemetry.json, or (B) update TELEMETRY.md to remove claims about installId and backend-generated env metadata and state that events only contain event name plus per-event allowlisted payload (keep GA4 note separate). Reference sanitizeEvent, TELEMETRY_EVENTS, and the emit/send function in node-backend/lib/telemetry/index.mjs when making the change.
🧹 Nitpick comments (1)
src/App.tsx (1)
200-227: ⚡ Quick winAvoid an
appApi.getGlobalSettings()round-trip per telemetry event.Every
telemetry-eventfrom the backend triggers a freshappApi.getGlobalSettings()call before forwarding to GA. For chatty events (e.g.installation.status_checked,provider.detected,agent.run.*), this adds an HTTP round-trip per event on the UI thread. Cache the enabled flag in a closure variable and refresh it only when telemetry settings actually change — e.g. by listening for thesettings.telemetry_changedtelemetry-event you already emit on the backend, or by reading settings once insetupGAand refreshing in the Settings save flow.♻️ Sketch
useEffect(() => { let unlisten: (() => void) | undefined; let gaInitialized = false; + let telemetryEnabled = true; const GA_QUEUE_KEY = 'productos_telemetry_queue'; @@ const setupGA = async () => { try { const settings = await appApi.getGlobalSettings(); - if (settings.telemetry?.enabled !== false) { + telemetryEnabled = settings.telemetry?.enabled !== false; + if (telemetryEnabled) { ReactGA.initialize('G-5L4YKT4HJV'); gaInitialized = true; await flushQueue(); } unlisten = await runtimeApi.listen<any>('telemetry-event', async (event) => { try { - const currentSettings = await appApi.getGlobalSettings(); - if (currentSettings.telemetry?.enabled !== false) { + const { event: name, payload } = event.payload; + if (name === 'settings.telemetry_changed' && typeof payload?.enabled === 'boolean') { + telemetryEnabled = payload.enabled; + } + if (telemetryEnabled) { if (!gaInitialized) { ReactGA.initialize('G-5L4YKT4HJV'); gaInitialized = true; + await flushQueue(); } - const { event: name, payload } = event.payload; if (navigator.onLine) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/App.tsx` around lines 200 - 227, The telemetry handler currently calls appApi.getGlobalSettings() on every runtimeApi.listen('telemetry-event') invocation; instead, introduce a cached boolean (e.g., telemetryEnabled) initialized once (read from appApi.getGlobalSettings() in setupGA or at component mount) and referenced inside the runtimeApi.listen callback (replace current appApi.getGlobalSettings() usage), and update that cached value only when telemetry settings change by subscribing to the 'settings.telemetry_changed' event (or by toggling it explicitly in the Settings save flow); keep existing symbols like runtimeApi.listen('telemetry-event'), gaInitialized, ReactGA.initialize and queueEvent unchanged, but remove the per-event getGlobalSettings() round-trip and use the cached telemetryEnabled flag inside the handler.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@TELEMETRY.md`:
- Around line 7-12: The docs claim telemetry includes a persisted installId and
backend-added env metadata, but the current implementation in
node-backend/lib/telemetry/index.mjs (notably sanitizeEvent and
TELEMETRY_EVENTS) strips all non-allowlisted keys and does not persist
installId; update either the code or the docs. To fix: either (A) reinstate
metadata by generating/persisting an installId and augmenting events in the emit
path before sanitizeEvent (identify the emit/send function in index.mjs and add
installId, ProductOS/version, runtime/node version, OS, arch) and ensure
persistence to telemetry/telemetry.json, or (B) update TELEMETRY.md to remove
claims about installId and backend-generated env metadata and state that events
only contain event name plus per-event allowlisted payload (keep GA4 note
separate). Reference sanitizeEvent, TELEMETRY_EVENTS, and the emit/send function
in node-backend/lib/telemetry/index.mjs when making the change.
---
Nitpick comments:
In `@src/App.tsx`:
- Around line 200-227: The telemetry handler currently calls
appApi.getGlobalSettings() on every runtimeApi.listen('telemetry-event')
invocation; instead, introduce a cached boolean (e.g., telemetryEnabled)
initialized once (read from appApi.getGlobalSettings() in setupGA or at
component mount) and referenced inside the runtimeApi.listen callback (replace
current appApi.getGlobalSettings() usage), and update that cached value only
when telemetry settings change by subscribing to the
'settings.telemetry_changed' event (or by toggling it explicitly in the Settings
save flow); keep existing symbols like runtimeApi.listen('telemetry-event'),
gaInitialized, ReactGA.initialize and queueEvent unchanged, but remove the
per-event getGlobalSettings() round-trip and use the cached telemetryEnabled
flag inside the handler.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0f90a555-787f-4f78-9ef0-815e9064a9cb
📒 Files selected for processing (4)
TELEMETRY.mdnode-backend/lib/telemetry/index.mjsnode-backend/server.mjssrc/App.tsx
…nt telemetry - Dynamically detect client OS and browser in GA4 configuration - Ingest global app version via appApi.getAppVersion() - Capture pageview hits on tab/document view changes - Trigger anonymous, privacy-compliant events: view.changed, file.opened, file.closed, file.edited, chat.message_sent, chat.opened, chat.closed
AssafMiron
approved these changes
May 21, 2026
This was referenced May 22, 2026
This was referenced Jun 14, 2026
This was referenced Aug 4, 2026
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
2 participants
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
Privacy safeguards
PRODUCTOS_TELEMETRY_DISABLED=1short-circuits before telemetry I/OVerification
node --test tests\\telemetry.test.mjsnode --check node-backend\\server.mjs; node --check node-backend\\lib\\workflows.mjs; node --check node-backend\\lib\\orchestrator.mjs; node --check node-backend\\lib\\telemetry\\index.mjsnpm run buildSummary by CodeRabbit
New Features
Documentation
Tests