devenv: add progress spinners for steps

[sandydoo]

Adds a spinner to steps like "Building shell" and friends.

TODOs

• Check fallback when not in a TTY
• Check that the alternative log formats still work

[claude]

Claude finished @sandydoo's task —— View job

---

Code Review: Progress Spinners Implementation

Todo List:

• Read and analyze main changed files (log.rs, devenv.rs, nix.rs)
• Review indicatif crate integration and dependency changes
• Check TTY detection and fallback behavior
• Assess thread safety and performance implications
• Review error handling and edge cases
• Evaluate test coverage
• Check code quality and best practices
• Provide comprehensive feedback

🔍 Code Review Findings

⚠️ Critical Issues

1. Thread Safety Risk: DevenvLayer::has_error uses global AtomicBool shared across spans, causing incorrect error attribution in concurrent operations.

   // In log.rs:421 - Error state affects all spans globally
   let has_error = self.has_error.load(Ordering::SeqCst);

   Fix: Use per-span error tracking in SpanContext.

2. Silent Error Handling: Mutex lock failures are ignored throughout DevenvIndicatifFilter.

   // log.rs:309 - Silent failure if lock acquisition fails
   if let Ok(mut spans) = self.user_message_spans.lock() {

   Fix: Log warnings on lock failures or use try_lock with timeout.

3. Potential Panic: Hardcoded template could panic if format changes.

   // log.rs:98 - Could panic on invalid template
   .unwrap()

   Fix: Handle template errors gracefully with fallback.

🚀 Performance Issues

1. Unnecessary Allocations: Multiple visitor structs created for identical string extraction tasks.
2. Lock Contention: Mutex<HashSet<span::Id>> could cause bottlenecks under high span creation rates.

📋 Missing Features from PR TODOs

From PR description:

• Check fallback when not in a TTY
• Check that the alternative log formats still work

Current status:

• ❌ TTY fallback: No explicit handling when IsTerminal() == false
• ❌ Alternative log formats: No verification that TracingFull/TracingPretty modes work correctly

🧪 Test Coverage Gap

Critical missing tests:

• TTY vs non-TTY behavior verification
• Concurrent span operations
• Error propagation correctness
• Alternative log format compatibility
• Spinner cleanup on span close

💡 Recommendations

1. Improve TTY Handling:

   // Add explicit TTY check in CLI mode
   if !stderr().is_terminal() {
       // Fall back to simple text output without indicators
   }

2. Fix Global Error State:

   // Add to SpanContext struct
   struct SpanContext {
       has_error: bool, // Per-span instead of global
       // ... other fields
   }

3. Add Error Handling:

   // Replace unwrap() with proper error handling
   let style = ProgressStyle::with_template("{spinner:.blue} {span_fields}")
       .unwrap_or_else(|_| ProgressStyle::default_spinner());

4. Extract Common Code:

   // Create shared utility for user message extraction
   fn extract_user_message(attrs: &span::Attributes) -> Option<String> { ... }

✅ Positive Aspects

• Clean integration with existing tracing infrastructure
• Good separation of concerns between different log formats
• Proper use of IsTerminal for TTY detection
• Well-structured field extraction pattern

🎯 Action Items

1. High Priority: Fix global error state issue (thread safety)
2. High Priority: Add comprehensive tests
3. Medium Priority: Improve error handling (remove unwraps)
4. Medium Priority: Verify alternative log formats work correctly
5. Low Priority: Optimize performance (reduce allocations)

The core implementation is solid, but the thread safety issues and missing tests need to be addressed before merging.

---

[cloudflare-workers-and-pages]

Deploying devenv with    Cloudflare Pages

Latest commit:	a64303f
Status:	✅  Deploy successful!
Preview URL:	https://4f4d0df7.devenv.pages.dev
Branch Preview URL:	https://indicatif-traces.devenv.pages.dev

View logs