From eaf9c6805009bf5edf9a4fbdbde0bb4a44c82a41 Mon Sep 17 00:00:00 2001
From: Bryan Helmkamp <bryan@brynary.com>
Date: Thu, 19 Mar 2026 14:55:25 -0400
Subject: [PATCH] Clean up telemetry: hoist shared computations, tighten
 visibility

- Compute sanitize_command, repository_identifier, and CI check once
  before the if/else branches to avoid duplicate git I/O
- Make should_track_for_level private (only used by _track_inner)
- Check tracks.is_empty() before credentials in upload_blocking for
  consistency with emit()

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
---
 lib/crates/fabro-cli/src/main.rs         | 15 +++++++++------
 lib/crates/fabro-telemetry/src/lib.rs    |  3 +--
 lib/crates/fabro-telemetry/src/sender.rs | 10 +++++-----
 3 files changed, 15 insertions(+), 13 deletions(-)

diff --git a/lib/crates/fabro-cli/src/main.rs b/lib/crates/fabro-cli/src/main.rs
index e0b24994a..6a4a9ea5a 100644
--- a/lib/crates/fabro-cli/src/main.rs
+++ b/lib/crates/fabro-cli/src/main.rs
@@ -386,23 +386,26 @@ async fn main() {
     let duration_ms = start.elapsed().as_millis() as u64;
 
     let is_error = result.is_err();
+    let command = fabro_telemetry::sanitize::sanitize_command(&raw_args, &command_name);
+    let repository = fabro_telemetry::git::repository_identifier();
+    let ci = std::env::var("CI").is_ok();
     if is_error {
         fabro_telemetry::track!("CLI Errored", {
             "subcommand": command_name,
-            "command": fabro_telemetry::sanitize::sanitize_command(&raw_args, &command_name),
+            "command": command,
             "durationMs": duration_ms,
-            "repository": fabro_telemetry::git::repository_identifier(),
-            "ci": std::env::var("CI").is_ok(),
+            "repository": repository,
+            "ci": ci,
             "success": false,
             "exitCode": 1,
         }, error);
     } else {
         fabro_telemetry::track!("CLI Executed", {
             "subcommand": command_name,
-            "command": fabro_telemetry::sanitize::sanitize_command(&raw_args, &command_name),
+            "command": command,
             "durationMs": duration_ms,
-            "repository": fabro_telemetry::git::repository_identifier(),
-            "ci": std::env::var("CI").is_ok(),
+            "repository": repository,
+            "ci": ci,
             "success": true,
             "exitCode": 0,
         });
diff --git a/lib/crates/fabro-telemetry/src/lib.rs b/lib/crates/fabro-telemetry/src/lib.rs
index 17ef30d6b..d494c4465 100644
--- a/lib/crates/fabro-telemetry/src/lib.rs
+++ b/lib/crates/fabro-telemetry/src/lib.rs
@@ -124,8 +124,7 @@ pub fn shutdown() {
     }
 }
 
-/// Check whether an event should be tracked given a telemetry level.
-pub fn should_track_for_level(level: TelemetryLevel, is_error: bool) -> bool {
+fn should_track_for_level(level: TelemetryLevel, is_error: bool) -> bool {
     match level {
         TelemetryLevel::Off => false,
         TelemetryLevel::Errors => is_error,
diff --git a/lib/crates/fabro-telemetry/src/sender.rs b/lib/crates/fabro-telemetry/src/sender.rs
index dd39ca14d..5015711c7 100644
--- a/lib/crates/fabro-telemetry/src/sender.rs
+++ b/lib/crates/fabro-telemetry/src/sender.rs
@@ -79,13 +79,13 @@ fn build_segment_batch(content: &str) -> Option<serde_json::Value> {
 /// on the background telemetry thread.
 /// No-ops if `SEGMENT_WRITE_KEY` was not set at compile time or `tracks` is empty.
 pub fn upload_blocking(tracks: &[Track]) -> anyhow::Result<()> {
-    let write_key = SEGMENT_WRITE_KEY
-        .ok_or_else(|| anyhow::anyhow!("SEGMENT_WRITE_KEY not set at compile time"))?;
-
     if tracks.is_empty() {
         return Ok(());
     }
 
+    let write_key = SEGMENT_WRITE_KEY
+        .ok_or_else(|| anyhow::anyhow!("SEGMENT_WRITE_KEY not set at compile time"))?;
+
     let lines: Vec<String> = tracks
         .iter()
         .filter_map(|t| serde_json::to_string(t).ok())
@@ -268,9 +268,9 @@ mod tests {
 
     #[test]
     fn upload_blocking_noops_with_empty_tracks() {
-        // With no write key, but empty tracks should still error on write key check
+        // Empty tracks returns Ok without checking credentials
         let result = upload_blocking(&[]);
-        assert!(result.is_err());
+        assert!(result.is_ok());
     }
 
     // -- Step 4: upload() tests --