feat: s2 --interactive (TUI for S2)

[infiniteregrets]

No description provided.

[greptile-apps]

Greptile Overview

Greptile Summary

Added a comprehensive Terminal User Interface (TUI) mode accessible via s2 --interactive, enabling interactive exploration and management of S2 basins, streams, and metrics. The implementation introduces ~14K lines of new code across the src/tui/ module with support for real-time tailing, append operations, metrics visualization, benchmarking, and a picture-in-picture mode for monitoring streams.

Major changes:

• Added TUI dependencies (ratatui, crossterm, chrono) and made CLI command optional to support interactive mode
• Implemented event-driven architecture with background tasks spawning for async operations (basin/stream listing, reading, metrics loading)
• Built comprehensive UI with multiple screens: basins list, streams list, stream details, read/tail view, append view, metrics view, access tokens, settings, and benchmark view
• Proper terminal cleanup with fallback error handling to prevent leaving user's shell in broken state
• Background tasks properly check for channel closure to exit when UI navigates away
• Calendar date picker for custom metrics time ranges with date validation

Issues noted:

• Multiple .expect() calls (22+ instances) that assume S2 client is initialized - could panic if state invariants are violated
• Date validation in calendar UI uses .expect() which could panic on invalid dates (as noted in previous comments on lines 6677, 6681)
• Spawned background tasks continue until completion or channel closure - no explicit cancellation mechanism for long-running operations beyond closing the event channel

Confidence Score: 4/5

• This PR is largely safe to merge with some minor concerns about error handling
• The implementation is well-structured with proper terminal cleanup and event handling, but contains multiple .expect() calls that could cause panics in edge cases. The core functionality is sound, spawned tasks properly check for channel closure, and the UI logic is comprehensive. The main risks are around date validation and assumptions about S2 client initialization state.
• Pay close attention to src/tui/app.rs - verify that S2 client is always initialized before operations that call .expect(), and test calendar date selection edge cases

Important Files Changed

Filename	Overview
src/main.rs	Added TUI module and interactive mode flag, properly handles fallback when command is missing
src/tui/mod.rs	Terminal setup and cleanup logic is robust with proper error handling and cleanup attempts even on failure
src/tui/app.rs	Core TUI logic with 7K+ lines - contains multiple .expect() calls that could panic, spawned tasks may continue after UI exit, and date validation issues noted in previous comments
src/tui/ui.rs	Extensive UI rendering logic - well-structured with helper functions, proper text truncation, and comprehensive styling

Sequence Diagram

sequenceDiagram
    participant User
    participant Main
    participant TUI as TUI Module
    participant App
    participant EventLoop
    participant Tasks as Background Tasks
    participant SDK as S2 SDK

    User->>Main: s2 --interactive
    Main->>Main: Parse CLI args
    Main->>TUI: run()
    TUI->>TUI: Load config
    TUI->>TUI: Setup terminal (raw mode, alt screen)
    TUI->>App: new(s2_client)
    App-->>TUI: App instance
    TUI->>App: run(terminal)
    
    App->>EventLoop: Start event loop
    App->>Tasks: load_basins(tx)
    Tasks->>SDK: list_basins()
    SDK-->>Tasks: Basin list
    Tasks->>EventLoop: Event::BasinsLoaded
    
    loop Main Loop
        EventLoop->>EventLoop: Draw UI
        EventLoop->>EventLoop: Poll keyboard input
        alt User input
            User->>EventLoop: Keypress
            EventLoop->>App: handle_key()
            App->>Tasks: Spawn background task
            Tasks->>SDK: API call
        end
        alt Background event
            Tasks->>EventLoop: Event (BasinsLoaded, RecordReceived, etc)
            EventLoop->>App: handle_event()
            App->>App: Update state
        end
        EventLoop->>EventLoop: Sleep 16ms
    end
    
    User->>EventLoop: Press 'q' to quit
    EventLoop->>App: Set should_quit
    App-->>TUI: Ok(())
    TUI->>TUI: Cleanup terminal (disable raw mode, restore screen)
    TUI-->>Main: Result
    Main-->>User: Exit

Loading

[greptile-apps]

_{2 files reviewed, 2 comments}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

Unwrapping calendar_start and calendar_end here assumes both are always Some when should_apply is true. Based on the logic at lines 6141-6155, calendar_end is only set when calendar_selecting_end is true AND the user makes a second selection. However, if the code path changes or there's a race condition, this could panic.

Suggested change

	let start_date = state.calendar_start.unwrap();
	let end_date = state.calendar_end.unwrap();
	let start_date = state.calendar_start.expect("calendar_start must be Some when should_apply is true");
	let end_date = state.calendar_end.expect("calendar_end must be Some when should_apply is true");

Prompt To Fix With AI

This is a comment left during a code review.
Path: src/tui/app.rs
Line: 6165:6166

Comment:
Unwrapping `calendar_start` and `calendar_end` here assumes both are always `Some` when `should_apply` is true. Based on the logic at lines 6141-6155, `calendar_end` is only set when `calendar_selecting_end` is true AND the user makes a second selection. However, if the code path changes or there's a race condition, this could panic.

```suggestion
                        let start_date = state.calendar_start.expect("calendar_start must be Some when should_apply is true");
                        let end_date = state.calendar_end.expect("calendar_end must be Some when should_apply is true");
```

How can I resolve this? If you propose a fix, please make it concise.

[greptile-apps]

The with_ymd_and_hms method returns a LocalResult<DateTime<Utc>> which could be None or Ambiguous for invalid dates. Unwrapping this could panic if a user selects an invalid date through the calendar UI (e.g., Feb 30).

Suggested change

	Utc.with_ymd_and_hms(year, month, day, 0, 0, 0).unwrap()
	} else {
	Utc.with_ymd_and_hms(year, month, day, 23, 59, 59).unwrap()
	};
	Utc.with_ymd_and_hms(year, month, day, 0, 0, 0)
	.single()
	.expect("invalid date selected in calendar")
	} else {
	Utc.with_ymd_and_hms(year, month, day, 23, 59, 59)
	.single()
	.expect("invalid date selected in calendar")

Prompt To Fix With AI

This is a comment left during a code review.
Path: src/tui/app.rs
Line: 6218:6221

Comment:
The `with_ymd_and_hms` method returns a `LocalResult<DateTime<Utc>>` which could be `None` or `Ambiguous` for invalid dates. Unwrapping this could panic if a user selects an invalid date through the calendar UI (e.g., Feb 30).

```suggestion
            Utc.with_ymd_and_hms(year, month, day, 0, 0, 0)
                .single()
                .expect("invalid date selected in calendar")
        } else {
            Utc.with_ymd_and_hms(year, month, day, 23, 59, 59)
                .single()
                .expect("invalid date selected in calendar")
```

How can I resolve this? If you propose a fix, please make it concise.

[infiniteregrets]

@greptile-apps review

[greptile-apps]

_{1 file reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

The inner .unwrap() on NonZeroU64::new(1) is unnecessary - 1 is always non-zero, so this will never panic, but the nested unwrap pattern is confusing

Suggested change

	NonZeroU64::new(target_mibps).unwrap_or(NonZeroU64::MIN),

_{Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!}

Prompt To Fix With AI

This is a comment left during a code review.
Path: src/tui/app.rs
Line: 6933:6933

Comment:
The inner `.unwrap()` on `NonZeroU64::new(1)` is unnecessary - `1` is always non-zero, so this will never panic, but the nested unwrap pattern is confusing

```suggestion
                NonZeroU64::new(target_mibps).unwrap_or(NonZeroU64::MIN),
```

<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>

How can I resolve this? If you propose a fix, please make it concise.