-
Notifications
You must be signed in to change notification settings - Fork 0
Add terminal info support with theme detection (v0.4.0) #5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Enables handlers to receive terminal information from clients for adaptive output formatting. Detects width, height, TTY status, color support, and theme (light/dark) using termbg. Breaking Changes: - CommandHandler::handle() now takes terminal_info parameter - Updated to v0.4.0 Features: - Terminal width/height detection (optional, can fail) - TTY detection (always available) - Color support levels: None, Basic16, Colors256, Truecolor - Theme detection (Light/Dark) via termbg with 100ms timeout - Detection happens client-side before each command - Graceful fallback when detection fails Dependencies: - termbg 0.6 - theme detection - terminal_size 0.4 - width/height detection - supports-color 3.0 - color capability detection Performance: - Theme detection: 10-25ms typical (validated on Terminal.app, Zed, Warp) - Fast failure: <1ms on unsupported terminals - Non-blocking: wrapped in spawn_blocking to protect async runtime 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
WalkthroughThis release introduces terminal capability detection to the daemon CLI architecture. A new Changes
Sequence DiagramsequenceDiagram
participant Client
participant TerminalModule as Terminal Detection
participant Transport
participant Server
participant Handler
Client->>TerminalModule: TerminalInfo::detect()
activate TerminalModule
TerminalModule->>TerminalModule: Detect width, height, TTY
TerminalModule->>TerminalModule: Detect color support
TerminalModule->>TerminalModule: Detect theme (blocking)
TerminalModule-->>Client: TerminalInfo
deactivate TerminalModule
Client->>Transport: Send Command { command, terminal_info }
activate Transport
Transport->>Transport: Serialize
Transport->>Server: SocketMessage
deactivate Transport
activate Server
Server->>Server: Deserialize → (command, terminal_info)
Server->>Server: Log terminal info
Server->>Handler: handle(command, terminal_info, output, cancel)
deactivate Server
activate Handler
Handler->>Handler: Process command
Handler-->>Server: Result
deactivate Handler
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (8)
CHANGELOG.md (2)
15-18: Document graceful fallback behavior for optional fields.The PR objectives mention "provides graceful fallbacks (fields optional when detection fails)," which is an important usability feature. Consider clarifying in the Added section that
TerminalInfofields are optional when detection fails, so handlers can implement defensive fallbacks gracefully.- Terminal info detection: width, height, TTY status, color support, theme (light/dark) - Fields are optional and gracefully omitted if detection fails - Handlers can safely unwrap or use defaults for missing fields - New types: `TerminalInfo`, `ColorSupport`, `Theme`
20-31: Expand migration guide with practical usage examples.The migration guide shows only the new function signature but doesn't explain how to work with
TerminalInfoor what capabilities it provides. Consider adding a brief example showing how handlers can access terminal metadata (e.g., color support, theme) to adapt output.### Migration Add `terminal_info` parameter to your handler (prefix with `_` if unused): ```rust async fn handle( &self, command: &str, terminal_info: TerminalInfo, // NEW - provides width, height, colors, theme output: impl AsyncWrite + Send + Unpin, cancel_token: CancellationToken, ) -> Result<i32>Example: Use terminal capabilities to adapt output:
async fn handle( &self, command: &str, terminal_info: TerminalInfo, output: impl AsyncWrite + Send + Unpin, cancel_token: CancellationToken, ) -> Result<i32> { if let Some(colors) = terminal_info.color_support { // Use color codes based on support level } if let Some(theme) = terminal_info.theme { // Adapt colors/styling to light or dark theme } Ok(0) }</blockquote></details> <details> <summary>examples/common/mod.rs (1)</summary><blockquote> `29-32`: **Example handler signature updated correctly for TerminalInfo** The `CommandHandler` impl now takes `_terminal_info: TerminalInfo`, which keeps the example compiling against the new API while explicitly marking the parameter as unused; the unknown-command `Ok(127)` branch remains unchanged behavior-wise and is a reasonable choice. If you want to showcase the new terminal features, consider a follow‑up that uses `terminal_info` here (e.g., printing detected width/theme) to give users a concrete example. Also applies to: 117-120 </blockquote></details> <details> <summary>examples/concurrent.rs (1)</summary><blockquote> `60-66`: **Concurrent example updated cleanly to the new CommandHandler API** Adding `_terminal_info: TerminalInfo` to `TaskQueueHandler::handle` aligns this example with the new trait without altering behavior, and the unknown-command `Ok(127)` path remains appropriate. You might later extend this example to actually use `terminal_info` (e.g., adjusting formatting when not a TTY) to demonstrate terminal-aware concurrency scenarios. Also applies to: 195-213 </blockquote></details> <details> <summary>src/lib.rs (1)</summary><blockquote> `35-41`: **Doc examples correctly updated for `terminal_info`** The CommandHandler examples now include the `terminal_info: TerminalInfo` parameter in the right position and match the trait signature. In the second (no_run) example you already use `_terminal_info`; if you want doctest users to avoid unused-variable warnings, you could optionally mirror that `_terminal_info` naming in the first and third examples as well. Also applies to: 214-222 </blockquote></details> <details> <summary>src/tests.rs (1)</summary><blockquote> `122-133`: **TerminalInfo wiring in handler tests is correct; consider small dedup** Both async tests now construct appropriate `TerminalInfo` instances and pass them into `handle`, aligning with the new trait signature and server behavior. If you find yourself adding more tests like this, you could factor a tiny helper (e.g., `fn basic_tty_info(...)`) to cut repetition, but it’s not strictly necessary at this size. Also applies to: 166-172, 177-179 </blockquote></details> <details> <summary>src/terminal.rs (1)</summary><blockquote> `88-104`: **Color detection and tests are minimal but effective; consider future extensibility** `detect_color_support()` correctly collapses the supports‑color levels into your four enums, and the tests ensure both it and `TerminalInfo::detect()` execute without panicking across environments. If you anticipate adding more color levels or themes later, you might consider marking the enums `#[non_exhaustive]` upfront to avoid breaking changes, but that’s a forward‑compatibility nicety rather than a requirement. Also applies to: 110-137 </blockquote></details> <details> <summary>tests/integration_tests.rs (1)</summary><blockquote> `271-283`: **Sequential-commands test correctly adapts to new connect API; optional helper extraction** Each iteration clones `daemon_exe` and calls `DaemonClient::connect_with_name_and_timestamp` with the same `build_timestamp` as the server, then asserts a `0` exit code. The logic is sound. If you ever want to trim repetition, consider a small helper that returns a connected `DaemonClient` for a given `(daemon_name, root_path, daemon_exe, build_timestamp)`, but this is optional for tests. </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: CodeRabbit UI **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between cf6414cc74b91bee6f44c74118294e2d34159a91 and 45981fa160e1b86f1281162bb225f3f9a9a14dbf. </details> <details> <summary>📒 Files selected for processing (12)</summary> * `CHANGELOG.md` (1 hunks) * `Cargo.toml` (2 hunks) * `examples/common/mod.rs` (2 hunks) * `examples/concurrent.rs` (2 hunks) * `src/client.rs` (3 hunks) * `src/lib.rs` (7 hunks) * `src/server.rs` (3 hunks) * `src/terminal.rs` (1 hunks) * `src/tests.rs` (5 hunks) * `src/transport.rs` (2 hunks) * `tests/integration_tests.rs` (16 hunks) * `tests/version_tests.rs` (9 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🧬 Code graph analysis (3)</summary> <details> <summary>src/client.rs (2)</summary><blockquote> <details> <summary>src/transport.rs (2)</summary> * `socket_path` (36-40) * `socket_path` (97-99) </details> <details> <summary>src/terminal.rs (1)</summary> * `detect` (51-85) </details> </blockquote></details> <details> <summary>tests/version_tests.rs (1)</summary><blockquote> <details> <summary>src/server.rs (1)</summary> * `new_with_name_and_timestamp` (140-162) </details> </blockquote></details> <details> <summary>tests/integration_tests.rs (2)</summary><blockquote> <details> <summary>src/server.rs (1)</summary> * `new_with_name_and_timestamp` (140-162) </details> <details> <summary>src/client.rs (1)</summary> * `connect_with_name_and_timestamp` (78-188) </details> </blockquote></details> </details> </details> <details> <summary>🔇 Additional comments (29)</summary><blockquote> <details> <summary>CHANGELOG.md (1)</summary><blockquote> `8-9`: **Verify release date is intentional.** The version header lists `2025-01-15` as the release date, but the PR was created on `2025-11-15`. Confirm whether this is a placeholder that should be updated to today's date or if an earlier release date is intended. </blockquote></details> <details> <summary>src/transport.rs (1)</summary><blockquote> `1-10`: **SocketMessage::Command payload change is consistent with new TerminalInfo flow** Switching `Command` to a struct variant with `{ command, terminal_info }` and importing `TerminalInfo` here matches the new client behavior and handler signatures; the framing and (de)serialization path remain unchanged. Please just confirm that `TerminalInfo` in `src/terminal.rs` derives `Serialize`/`Deserialize` (or otherwise implements them) and that server-side matching is updated to: ```rust SocketMessage::Command { command, terminal_info }everywhere, to avoid subtle protocol mismatches.
Also applies to: 174-177
src/client.rs (2)
2-4: Client imports and daemon spawn call remain straightforwardImporting
TerminalInfoalongsidesocket_path,SocketClient, andSocketMessageis consistent with the new protocol usage, and the minor reflow of thespawn_and_wait_for_readycall doesn’t change behavior.Just ensure all call sites use the same
spawn_and_wait_for_readysignature and that no downstream code still expects the oldSocketMessage::Commandshape.Also applies to: 112-118
310-319: TerminalInfo detection and propagation before command execution looks correctDetecting terminal info in
execute_command, logging it, and then sendingSocketMessage::Command { command, terminal_info }cleanly wires the new metadata into the existing streaming loop without altering output semantics.Two points to sanity‑check:
- Detection cost –
TerminalInfo::detect().awaitruns per command. The design doc says 10–25 ms typical; if you ever see slower terminals, you could cacheTerminalInfoinDaemonClientand refresh it only on demand.- Protocol expectations – Verify the server
SocketMessagematch arm is now:SocketMessage::Command { command, terminal_info }and that
TerminalInfoisSerialize + DeserializeandDebug, matching the logging and serialization here.Also applies to: 321-327
Cargo.toml (1)
3-3: Verification complete: no security advisories foundNo security advisories were found for termbg 0.6 or terminal_size 0.4. The supports-color Rust crate (3.0) has no RustSec advisory (the npm package incident is unrelated). The version bump and dependency additions are safe to merge.
Regarding default features: all three crates use defaults (no explicit feature restrictions). This is a valid design choice, though if binary size is a concern, you could selectively disable features for individual crates—but this is optional optimization, not required.
src/lib.rs (2)
109-116: Public re‑exports of terminal types look consistentAdding
mod terminal;pluspub use terminal::{ColorSupport, TerminalInfo, Theme};and wiring them throughpreludekeeps the public API coherent and ergonomic for downstream users.Also applies to: 124-127
258-262:CommandHandlertrait extension withterminal_infois soundThe new documentation block for
terminal_infoclearly explains semantics and failure modes, and the updated signature keeps argument order intuitive (&strthenTerminalInfothenoutputandcancel_token). This matches how the server and tests are invoking it elsewhere.Also applies to: 268-275
src/tests.rs (2)
17-29: Test handler trait implementation updated correctly
TestHandler’shandlenow accepts_terminal_info: TerminalInfoin the correct position and ignores it, which keeps the compilation check meaningful without changing test behavior.
55-65: Good coverage ofSocketMessage::CommandwithTerminalInfoThe serialization test now constructs a non‑trivial
TerminalInfoand asserts every field after round‑trip JSON (includingcolor_supportandtheme), which is a solid regression guard for the transport change.Also applies to: 69-78
tests/version_tests.rs (3)
17-29: SimpleHandler updated to new CommandHandler signatureThe added
_terminal_info: TerminalInfoargument matches the library trait and is safely ignored for these version‑handshake tests.
38-45: Server construction formatting‑only changesThe reflowed
DaemonServer::new_with_name_and_timestampcalls are purely stylistic and don’t affect behavior; argument order and values remain consistent.Also applies to: 83-90, 135-141, 185-191, 244-250, 293-299
211-223: Command tests now correctly include terminal metadata
test_version_handshake_before_commandandtest_command_without_handshake_failsnow sendSocketMessage::Command { command, terminal_info }with realistic TerminalInfo instances. This keeps the protocol tests aligned with the new transport shape and ensures the server path is exercised with both TTY and non‑TTY configurations.Also applies to: 261-272
src/server.rs (2)
45-53: Server‑side CommandHandler example kept in syncThe example implementation now includes
_terminal_info: TerminalInfoin the right slot, matching the public trait and making it clear to users where terminal context is injected.
290-305: TerminalInfo propagation from transport to handler looks correctThe server now:
- Deserializes
SocketMessage::Command { command, terminal_info },- Logs terminal characteristics at debug level, and
- Invokes
handler.handle(&command, terminal_info, output_writer, cancel_token_clone).This is a clean, single‑ownership flow for
TerminalInfoand integrates smoothly with the existing streaming/cancellation logic.Also applies to: 316-319
src/terminal.rs (1)
5-18: TerminalInfo data model and detection strategy are well‑structuredThe
TerminalInfo/ColorSupport/Themetypes map cleanly onto terminal capabilities, derive the right traits for transport (Serialize/Deserialize, Clone, Eq), andTerminalInfo::detect()composes:
- TTY and size checks,
- Color level via
supports_color, and- Theme via a bounded
spawn_blockingcall totermbg(gated onis_tty),with graceful fallbacks when any step fails. This gives handlers a robust, low‑friction snapshot of the client terminal.
Also applies to: 21-38, 40-85
tests/integration_tests.rs (14)
28-34: Server helper uses new connection-limit-aware constructor correctly
start_test_daemonnow callsDaemonServer::new_with_name_and_timestampwith the expected(daemon_name, root_path, build_timestamp, handler, 100)parameter order and a generous connection limit for tests. This matches the server constructor signature and keeps existing test behavior intact.
53-59: Custom connection-limit helper is wired correctly
start_test_daemon_with_limitforwards themax_connectionsargument directly intoDaemonServer::new_with_name_and_timestamp, which is exactly what the connection-limit tests rely on. The parameter ordering and types look correct.
81-89: EchoHandler updated to accept TerminalInfo without behavioral changesAdding
_terminal_info: TerminalInfoaftercommand: &strkeeps the signature aligned with the updatedCommandHandlertrait while leaving handler behavior unchanged. Using a leading underscore for the unused parameter is idiomatic.
101-109: ChunkedHandler signature now matches CommandHandlerThe inserted
_terminal_info: TerminalInfoparameter is in the correct position and type, and the chunked output loop remains unchanged, so the test still exercises streaming behavior as before.
124-143: CancellableHandler updated for TerminalInfo; cancellation semantics preservedThe new
_terminal_info: TerminalInfoargument is correctly added, and the cancellation loop logic is untouched. The handler still returns an error on cancellation as expected by the tests.
149-161: ErrorHandler signature updated cleanly for TerminalInfoThe
_terminal_info: TerminalInfoparameter is correctly placed between_commandandoutput, matching the trait. Error propagation (Err(anyhow!("Test error"))) is preserved.
169-190: Basic streaming test migrated to new client connect API and stronger exit-code check
DaemonClient::connect_with_name_and_timestamp(&daemon_name, &root_path, daemon_exe, build_timestamp)matches the client constructor’s signature and uses the samebuild_timestampas the server, ensuring the version handshake passes. The addedassert_eq!(result.unwrap(), 0)makes the test stricter without changing intent.
203-220: Chunked-output test updated to use versioned connect and explicit success codeThe
connect_with_name_and_timestampcall passes consistent daemon metadata, and the newassert_eq!(result.unwrap(), 0)clearly asserts a successful exit code in addition tois_ok(). This is a solid tightening of the test.
233-245: Error-reporting test uses new connect API consistently with server setup
connect_with_name_and_timestampreceives the samebuild_timestampand path used to launch the server, so the version handshake will succeed before exercising the error path. The core assertions on error propagation remain unchanged.
304-312: Connection-close test now uses the versioned connect API appropriatelyCreating a single mutable client with
connect_with_name_and_timestampand then moving it into the spawned task preserves the long-running command scenario while respecting the new version handshake semantics.
352-386: ConcurrentTrackingHandler updated to accept TerminalInfo while preserving concurrency trackingThe
_terminal_info: TerminalInfoparameter is correctly added to the signature, and the active/max concurrency bookkeeping remains unchanged. This keeps the handler compatible with the new trait without impacting the concurrency assertions.
401-429: Concurrent-clients test uses new connect API per task and tightens exit-code validationEach spawned task calls
connect_with_name_and_timestampwith cloned daemon metadata and the sharedbuild_timestamp, and the nested assertions (result.is_ok(),exit_code.is_ok(), andexit_code.unwrap() == 0) clearly separate task panics, client errors, and non-zero exit codes. This is a good, explicit check.
461-472: Stress test migrated to new connect API with intact success/error accountingAll 15 concurrent clients now connect via
connect_with_name_and_timestampusing the same timestamp as the server. The subsequent match onOk(exit_code)vs.Err(_)still correctly distinguishes successful runs from expected errors in the stress scenario.
529-543: Connection-limit test correctly targets limited server via new client connect APIEach of the 6 clients uses
connect_with_name_and_timestampwith consistent daemon metadata, which is crucial for exercising the non-blocking semaphore behavior introduced in the server. The rest of the test logic continues to validate that only up to 3 executions succeed concurrently.
Summary
Adds terminal information detection to enable handlers to adapt their output based on the client's terminal capabilities.
Breaking Changes
terminal_infoparameter.See CHANGELOG.md for migration guide.
Features
spawn_blockingDependencies
Migration
Add
terminal_infoparameter to your handler:Test Plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Breaking Changes
New Features
Chores