Repository navigation
feat(tui): add terminal dashboard for SMG - #867
Merged
Merged
Conversation
github-actions
Bot
added
documentation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new terminal UI crate Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Main as main.rs
participant App as app.rs
participant EventHandler as event.rs
participant SmgClient as client.rs
participant Gateway as SMG Gateway
User->>Main: run smg-tui
Main->>SmgClient: new(gateway_url, metrics_url, api_key)
Main->>Gateway: check_alive()
alt gateway unreachable and --auto-start
Main->>Main: spawn smg gateway process
Main->>Gateway: poll readiness (timeout)
end
Main->>App: App::new(state, client)
Main->>EventHandler: spawn event loop
Main->>App: run()
loop UI loop
EventHandler->>App: AppEvent (Key/Tick/Resize)
App->>SmgClient: HTTP commands / list / actions
App->>App: render frame (ui/mod)
end
loop Polling task
Poller->>SmgClient: list_workers, get_loads, metrics, models
Poller->>SharedState: update caches and histories
end
sequenceDiagram
participant User
participant App as app.rs
participant Chat as chat.rs
participant SmgClient as client.rs
participant Gateway as /v1/chat or /v1/responses
User->>App: Enter message
App->>Chat: stream_chat(...)
Chat->>SmgClient: stream_request(path, body)
SmgClient->>Gateway: POST (stream:true)
Gateway->>SmgClient: SSE stream (data: ...)
SmgClient->>Chat: stream bytes
Chat->>App: send tokens via mpsc channel
App->>App: drain channel, append assistant text, render updates
User->>App: Esc (cancel)
App->>Chat: drop/stop streaming receiver
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
|
Hi @key4ng, the DCO sign-off check has failed. All commits must include a To fix existing commits: # Sign off the last N commits (replace N with the number of unsigned commits)
git rebase HEAD~N --signoff
git push --force-with-leaseTo sign off future commits automatically:
|
Contributor
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a comprehensive terminal UI for the Shepherd Model Gateway, addressing the need for a visual interface to monitor workers, manage routing, and interact with models. The Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 263b44d602
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This is an impressive addition, introducing a full-featured terminal UI for SMG. The implementation is comprehensive, covering real-time monitoring, worker management, and an interactive chat playground. The code is well-structured, and features like gateway auto-start and automatic GPU management are excellent. My review focuses on improving robustness in a few key areas, such as metrics parsing and error handling, along with some minor performance and maintainability suggestions. Overall, this is a fantastic new capability for the project. I've added a suggestion to log errors instead of panicking in one instance.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 30
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tui/README.md`:
- Around line 66-70: The fenced code blocks showing the metrics snapshot and the
keybinding examples lack language tags and trigger MD040; update each of those
triple-backtick blocks (the metrics snapshot block and the other examples
referenced) to use the "text" language tag (i.e., change ``` to ```text) so
markdownlint stops flagging them; look for the blocks containing the
WORKERS/CIRCUIT BREAKERS table and the keybinding examples and add the "text"
tag to each fenced block.
- Around line 147-149: The README currently refers to both `/v1/chat` and
`/v1/chat/completions`; choose one canonical chat endpoint (either `/v1/chat` or
`/v1/chat/completions`) and make all references consistent: update the
"Multi-turn conversation" bullet and the TUI description so they use the same
endpoint string, and ensure any related examples, links, or headings (e.g.,
mentions of Chat completions or Responses API) are adjusted to match that
canonical endpoint; search for occurrences of `/v1/chat` and
`/v1/chat/completions` and replace them so only the chosen form remains.
In `@tui/src/app.rs`:
- Around line 1018-1031: spawn_local_worker currently binds a TcpListener to
"127.0.0.1:0" to read an ephemeral port then immediately drops it, causing a
TOCTOU race; change the implementation to avoid dropping the bound listener
before the worker is listening by either (1) passing port 0 to the worker
runtime (so the worker itself binds an ephemeral port and returns the actual
port), or (2) keep the TcpListener open and hand the listener or its bound
address to the spawned process (or retry binding when starting the worker) so
the port is not reclaimed between the bind and worker start; update
spawn_local_worker, the use of TcpListener and any logic around port and
set_status to implement one of these approaches.
- Around line 1071-1073: The code uses log_file.try_clone().unwrap() to create
log_file2 which can panic; replace the unwrap with explicit error handling
around try_clone() (e.g., match or Result propagation) so a failure to duplicate
the file is handled gracefully: call log_file.try_clone(), on Ok assign to
log_file2, on Err log an error via the same logger/UI and either fall back to a
single writer or return an Err from the enclosing function (propagate the
Result) so the TUI can display a friendly message instead of panicking; update
the function signature if needed to return Result and reference the try_clone
call and the log_file2 variable when making the change.
- Around line 745-760: The code uses Option::is_none_or on self.active_filter
(in the filter closure over wl.workers) which requires Rust 1.82+, but the
project targets 1.75. Replace the is_none_or usage with an equivalent map_or
call: on self.active_filter use map_or(true, |f| { ... }) so the closure becomes
self.active_filter.map_or(true, |f|
w.id.to_lowercase().contains(&f.to_lowercase()) ||
w.url.to_lowercase().contains(&f.to_lowercase())); keep the rest of the logic
that builds filtered, uses self.selected_index, and sets self.confirm_delete to
Some((worker.id.clone(), worker.url.clone())) unchanged.
In `@tui/src/chat.rs`:
- Around line 124-182: The final aggregated response is being sent even when
delta chunks were already emitted, causing duplicate output; add a flag (e.g.,
saw_deltas) scoped near the loop and set it to true inside the
"response.output_text.delta" branch, then in the "response.completed" |
"response.done" branch skip sending the full aggregated text if saw_deltas is
true (still send the "\n[DONE]" marker and return); update the handling around
parsed/event_type and tx sends (the buffer/stream loop, the
"response.output_text.delta" branch, and the
"response.completed"/"response.done" branch) to reference this flag.
In `@tui/src/client.rs`:
- Around line 13-14: Remove the incorrect #[allow(dead_code)] attribute applied
to the metrics_url field; metrics_url is actually used by fetch_metrics(), so
delete the #[allow(dead_code)] line above the metrics_url declaration to allow
the compiler to accurately reflect usage and avoid hiding potentially useful
warnings for that field.
- Around line 256-270: The current stream_request builds a new reqwest::Client
(stream_client) per call; instead add a dedicated streaming client field (e.g.,
streaming_client: reqwest::Client) initialized once in the client's
constructor/builder with the longer timeout (120s) and any TLS/config needed,
then change stream_request to use self.streaming_client.post(&url) instead of
creating a new client; alternatively, if you prefer not to add a field, use the
existing shared client and set a per-request timeout via request.timeout(...)
before send(). Ensure you preserve the bearer_auth logic (if let Some(key) =
&self.api_key) and keep error_for_status() handling.
In `@tui/src/event.rs`:
- Around line 41-45: The event handler currently forwards every Event::Key to tx
which can double-fire actions; guard the send with a check for key.kind ==
KeyEventKind::Press (import KeyEventKind from crossterm::event) and only call
tx.send(AppEvent::Key(key)) when the key event kind is Press, otherwise ignore
the event; keep the existing error/is_err() break behavior when sending fails.
In `@tui/src/main.rs`:
- Around line 156-169: The setup currently enables raw mode and switches to the
alternate screen before installing the panic hook, so if any early fallible call
(e.g., execute!(..., EnterAlternateScreen) or Terminal::new(...)) returns Err
via `?` the terminal is left mutated; wrap the terminal mutations in an RAII
guard struct (e.g., TerminalGuard) created immediately after enable_raw_mode()
that performs disable_raw_mode() and leaves the alternate screen in its Drop
impl and provide a disarm method to cancel cleanup on successful explicit
teardown; install the panic hook to call the guard's cleanup path (or rely on
Drop) and ensure creation of Terminal via Terminal::new(...) happens while the
guard is active so any error triggers Drop to restore the terminal state.
In `@tui/src/state.rs`:
- Around line 93-101: spawn_poller currently spawns a background task and drops
the JoinHandle, preventing graceful cancellation; change spawn_poller to return
the tokio::task::JoinHandle<()> (or a typed handle) instead of returning
nothing, create the handle with let handle = tokio::spawn(async move { ... });
return that handle, and ensure callers of spawn_poller capture the returned
JoinHandle and call abort().await or handle.await during shutdown; keep the
internal loop and poll_once usage unchanged (referencing spawn_poller and
poll_once) so callers can cancel the poller cleanly.
- Around line 394-407: The subtraction cur_sum - prev_sum can be negative after
a Prometheus counter reset; update the logic in the block using
parse_duration_stats and the s.prev_duration_sum / s.prev_duration_count guards
so delta_sum is clamped to non-negative (e.g., set delta_sum = if cur_sum >=
prev_sum { cur_sum - prev_sum } else { 0.0 }) before computing avg_latency and
pushing into s.avg_latency_history (respecting SPARKLINE_CAP); keep the existing
saturating_sub for delta_count and the conditional avg calculation.
- Around line 352-374: The throughput_history is being pushed twice per poll
(once with total_throughput and again with rps); change the logic so
throughput_history is only updated from rps (Prometheus) when metrics are
available and a prev_request_count exists, otherwise fall back to
total_throughput. Concretely, update the block that pushes total_throughput
(s.throughput_history.push_back(total_throughput)) to only execute when metrics
is Err or when parse_request_count/prev_request_count cannot produce rps, and
keep the push of rps (computed from parse_request_count and prev_request_count)
as the primary update; reference s.throughput_history, total_throughput, rps,
parse_request_count, and s.prev_request_count to locate and guard the duplicate
push.
In `@tui/src/types.rs`:
- Around line 16-69: The three View helpers (View::from_key, View::index,
View::all) are manually duplicated and can drift; refactor to a single
source-of-truth static ALL slice (e.g., pub const ALL: &[View]) and implement
from_key and index in terms of that slice (compute position for index and lookup
by digit-derived index in from_key) and have label either derived from a
parallel NAME slice or from a method on each variant but referenced via ALL;
update View::all to return that constant and add a small unit test that
validates ALL.len() and that index/from_key round-trip to catch future
mismatches.
- Around line 268-276: The env_key() method always returns Some(...) for every
variant (Self::OpenAI, Self::Anthropic, Self::Xai, Self::Gemini), so change its
signature from pub fn env_key(&self) -> Option<&'static str> to pub fn
env_key(&self) -> &'static str and update the match arms to return the string
literals directly (e.g., Self::OpenAI => "OPENAI_API_KEY"); then search for and
update all call sites that expect an Option (remove unwraps or Option handling)
to use the plain &str return.
In `@tui/src/ui/action_menu.rs`:
- Around line 136-149: The SelectModel menu currently leaks heap by calling
Box::leak for per-frame numbering strings when building items (see the items
vector creation and custom_num); update the code to stop leaking by passing
owned Strings (or Cow<'static, str>) into render_menu instead of &str, or keep
indices as numeric types and format them inside a stable, non-leaking buffer.
Concretely, change the items type from Vec<(&str, String, String)> and the refs
conversion to pass owned String/Cow (or Vec<(String, String, String)>) and
update render_menu signature to accept owned Strings/Cow<'static, str>, or
alternatively keep numbers as usize and format them on-demand in render_menu;
adjust calls in render_add_menu / render_menu and the title construction
(runtime.label()) accordingly so no Box::leak is used per frame.
In `@tui/src/ui/detail.rs`:
- Around line 121-127: The UI currently derives circuit breaker text/color from
worker.is_healthy (cb_label/cb_color), which is incorrect; change the code that
sets cb_label and cb_color to read the actual breaker state from the shared
breaker/metrics state instead of worker.is_healthy — e.g., call the service that
exposes breaker state (shared_metrics.get_breaker_state(worker.id) or
worker.breaker_state) and map that value to "open"/"closed" and
theme::RED/GREEN, then use those variables in the existing
lines.push(Line::from(...)) Span::styled calls so the detail panel reflects the
real circuit-breaker status.
- Around line 199-204: truncate_str currently slices bytes
(&s[..max.saturating_sub(1)]) which panics on multi-byte UTF-8 boundaries;
change it to operate on chars: check s.chars().count() <= max and return
s.to_string() otherwise take max.saturating_sub(1) chars via
s.chars().take(...).collect::<String>() and append the ellipsis; also handle the
edge case when max == 0 (return empty string or just the ellipsis per desired
behaviour). Use the function name truncate_str to locate and replace the
byte-slicing logic with char-based truncation.
In `@tui/src/ui/dialog.rs`:
- Around line 11-50: The popup centering/Layout code is duplicated between
render_delete_dialog and render_flush_dialog; extract a small helper (e.g., fn
centered_popup(area: Rect, width: u16, height: u16) -> Rect or fn popup_rect(f:
&mut Frame, width: u16, height: u16) -> Rect) that encapsulates the
Layout::vertical and Layout::horizontal + Constraint::Length/Flex::Center logic
used to compute popup, then replace the duplicated blocks in
render_delete_dialog and render_flush_dialog to call that helper with the
desired width/height and use its return value when rendering the Clear and
Paragraph widgets.
- Around line 40-43: The code uses tuple indexing (info.0, info.1) when building
the confirmation text in dialog.rs (used in confirm_delete and confirm_flush),
making it hard to read; replace tuple access with either a small named struct
(e.g., WorkerInfo { id, url }) or destructure the tuple at the call site (let
(id, url) = info) and then use id and url in the format! call; update the
related occurrences around confirm_delete and confirm_flush to use the new names
for clarity and consistency.
In `@tui/src/ui/footer.rs`:
- Around line 25-37: Update the footer view-range hint from "1-5" to "1-7" so
the footer advertises all seven views; locate the two occurrences of hint("1-5",
"view") in the footer construction (the match arms that build the Vec of hints
using the hint(...) calls) and change them to hint("1-7", "view") to match the
README and the newly added Traffic and Mesh tabs.
In `@tui/src/ui/help.rs`:
- Around line 36-44: The duplicated "Navigation" header in the help text (inside
the match arm that calls text.push_str with the block starting "Navigation\n j
/ Down...") should be renamed to something like "Selection" or "List Navigation"
to avoid confusion with the earlier view-switching "Navigation" section; locate
the text.push_str call in help.rs that contains the "Navigation" literal and
update that header string to "Selection" (or "List Navigation") while preserving
the rest of the formatting and shortcuts.
In `@tui/src/ui/logs.rs`:
- Around line 170-197: render_file_log currently does blocking file I/O
(std::fs::read_to_string and content.lines()) on the render path which stalls
the UI; move all file reading and line-walking into a background task that tails
or caches the last N lines and expose a non-blocking accessor on App (e.g.,
App::cached_log(path) or a LogsCache component) so render_file_log only reads
that cached slice and renders it; ensure the background poll updates the cache
atomically and handles file-not-found/errors so render_file_log can simply
render a precomputed Vec<Line> or a fallback message without performing any disk
I/O.
In `@tui/src/ui/models.rs`:
- Line 16: The table header "OWNER" in the Row::new(...) call does not match the
value being rendered (m.display_name); update the header in Row::new(vec!["ID",
"OWNER", "WORKERS", "CREATED"]) to reflect the actual displayed field (e.g.,
"DISPLAY NAME") or instead render the true owner field (replace m.display_name
with m.owner or the correct owner accessor) so the column label and value align;
adjust the header or the cell rendering in the models table accordingly.
In `@tui/src/ui/pulse.rs`:
- Around line 20-37: The narrow-layout branch currently returns before calling
render_request_stats, so add the request-stats panel to the vertical layout:
extend the constraints Vec (used with Layout::vertical) to include an extra
Constraint::Fill(1) for request stats, adjust the subsequent pushes/order to
keep Worker Health, optional Node Status (has_node_panel), Throughput, and
Request Stats, then after splitting into rows call render_worker_health(...),
optionally render_node_status(...), render_throughput_compact(...), and finally
render_request_stats(f, &state, rows[i]) (incrementing i as you go) before
returning; update the rows indexing to match the added constraint.
In `@tui/src/ui/stats_bar.rs`:
- Around line 82-89: The health_text logic incorrectly shows "all healthy" when
total == 0; update the branch in the health_text computation (referencing total,
healthy, unhealthy, state.connected, and health_text) to first handle total == 0
(and when connected) by returning a neutral label like "no workers" or "--" with
theme::TEXT_MUTED, then proceed to the existing unhealthy == 0 => "all healthy"
(theme::GREEN) and else => "{unhealthy} unhealthy" (theme::RED); ensure the new
check is before the unhealthy == 0 branch so empty gateways are not reported as
all healthy.
In `@tui/src/ui/tabs.rs`:
- Around line 14-32: The tab numbering uses the iterator index i + 1 but should
use the View-provided 1-based index for consistency; replace the use of i + 1
when building the num string with view.index() in the tabs construction (the
block that builds tabs: Vec<Span> in tabs.rs), i.e., change the num assignment
to use view.index() so Span::styled(format!("{num}:{label}"), ...) uses the
authoritative View::index() value.
In `@tui/src/ui/workers.rs`:
- Around line 321-341: get_worker_load_info currently only reads
details.loads.first(), dropping additional TP>1 entries and under-reporting
running/token usage, and also treats plain http:// workers as 0 when /get_loads
is missing; update get_worker_load_info to aggregate across all entries in
wl.details.loads (summing num_running_reqs and computing a weighted or averaged
token_usage consistent with the roll-up in state.rs), return the aggregated
running and usage strings, and if no loads are present fall back to per-worker
Prometheus rps from worker_rps for any non-load-backed URL (use
worker_url.starts_with checks as currently implemented for grpc/https but
include plain http:// fallback to worker_rps instead of returning "0"). Ensure
you update references to wl.details, details.loads, worker_rps, and worker_url
in get_worker_load_info so all load entries are summed rather than only using
the first.
- Around line 264-277: The table selection is clamped when building table_state
but the detail pane still uses the raw app.selected_index; compute a single
clamped index once (e.g., let clamped = if row_count==0 { None } else {
Some(app.selected_index.min(row_count.saturating_sub(1))) }) and use that both
for table_state.select(...) and for indexing into filtered before calling
detail::render_detail so the detail pane always matches the highlighted row and
avoids out-of-bounds access.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4cdb3e65-d847-4094-a825-23b5d636e80a
⛔ Files ignored due to path filters (2)
tui/assets/add-worker.gifis excluded by!**/*.giftui/assets/tui-demo.gifis excluded by!**/*.gif
📒 Files selected for processing (27)
Cargo.tomltui/Cargo.tomltui/README.mdtui/src/app.rstui/src/chat.rstui/src/client.rstui/src/event.rstui/src/lib.rstui/src/main.rstui/src/state.rstui/src/types.rstui/src/ui/action_menu.rstui/src/ui/chat.rstui/src/ui/detail.rstui/src/ui/dialog.rstui/src/ui/filter.rstui/src/ui/footer.rstui/src/ui/help.rstui/src/ui/logs.rstui/src/ui/mod.rstui/src/ui/models.rstui/src/ui/pulse.rstui/src/ui/sparkline.rstui/src/ui/stats_bar.rstui/src/ui/tabs.rstui/src/ui/theme.rstui/src/ui/workers.rs
key4ng
force-pushed
the
keyang/tui-redesign
branch
from
March 22, 2026 20:00
263b44d to
50b7078
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 50b7078996
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
key4ng
force-pushed
the
keyang/tui-redesign
branch
from
March 22, 2026 20:11
50b7078 to
95a1ee0
Compare
There was a problem hiding this comment.
Actionable comments posted: 14
♻️ Duplicate comments (23)
tui/src/client.rs (1)
264-266: 🧹 Nitpick | 🔵 TrivialAvoid creating a new
reqwest::Clientper streaming request.Each call to
stream_requestbuilds a newreqwest::Client, losing connection pooling benefits and TLS session cache. Consider initializing a dedicated streaming client with the longer timeout in the constructor.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tui/src/client.rs` around lines 264 - 266, The streaming code builds a new reqwest::Client inside stream_request which prevents connection pooling and TLS session reuse; instead add a dedicated streaming client field (e.g., stream_client: reqwest::Client) to the client struct initialized in the constructor using Client::builder().timeout(Duration::from_secs(120)). Replace the local builder call in stream_request with a reuse of self.stream_client so all streaming requests reuse the same client and timeout configuration.tui/src/state.rs (3)
395-398:⚠️ Potential issue | 🟠 MajorBug:
throughput_historyreceives duplicate entries per poll cycle.When both worker metrics are available (line 398) AND Prometheus metrics succeed with a previous count (line 427),
throughput_historygets pushed twice per cycle. This corrupts sparkline data.Based on the comment at line 423,
rpsshould be the primary source. Consider removing the push at line 398 or making them mutually exclusive.Also applies to: 424-427
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tui/src/state.rs` around lines 395 - 398, throughput_history is being appended twice per poll cycle because total_throughput is pushed unconditionally and later rps (from Prometheus) can also push; update the logic so the sparkline gets a single source per cycle: prefer rps when available by making the first push conditional (e.g., only push total_throughput into s.throughput_history when rps is not available) or convert the two pushes into mutually exclusive branches; touch the block that uses SPARKLINE_CAP and total_throughput as well as the later branch that pushes rps to ensure only one push to s.throughput_history happens per cycle.
95-105: 🧹 Nitpick | 🔵 TrivialConsider returning
JoinHandlefor graceful shutdown.
spawn_pollerdiscards theJoinHandle, preventing the caller from canceling the poller during application shutdown.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tui/src/state.rs` around lines 95 - 105, spawn_poller currently drops the tokio::task::JoinHandle which prevents graceful shutdown; change spawn_poller to return the JoinHandle (tokio::task::JoinHandle<()>) instead of () by capturing the result of tokio::spawn and returning it, update callers to hold and abort/await the handle during shutdown, and ensure the signature in state.rs (spawn_poller) and any call sites are updated to propagate or store the returned JoinHandle so the poller can be cancelled or awaited cleanly.
451-466:⚠️ Potential issue | 🟡 MinorPotential negative latency on Prometheus counter reset.
Line 453 uses plain subtraction (
cur_sum - prev_sum) which can yield a negative value if Prometheus restarts and counters reset. This would push negative latency values toavg_latency_history.Proposed fix
let (cur_sum, cur_count) = parse_duration_stats(&metrics_text); if let (Some(prev_sum), Some(prev_count)) = (s.prev_duration_sum, s.prev_duration_count) { - let delta_sum = cur_sum - prev_sum; + let delta_sum = if cur_sum >= prev_sum { cur_sum - prev_sum } else { 0.0 }; let delta_count = cur_count.saturating_sub(prev_count);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tui/src/state.rs` around lines 451 - 466, The subtraction cur_sum - prev_sum in the parse_duration_stats handling can produce negative delta_sum when Prometheus resets counters; update the logic in the block around parse_duration_stats / s.prev_duration_sum / s.prev_duration_count so you compute delta_sum = (cur_sum as f64 - prev_sum as f64) and then guard: if delta_sum > 0.0 && delta_count > 0 { avg_latency = delta_sum / delta_count as f64; push to s.avg_latency_history (evict oldest when >= SPARKLINE_CAP) } else skip pushing (or treat avg_latency as 0.0 but do not push negative values). Ensure prev_duration_sum and prev_duration_count are still updated after the check.tui/src/ui/tabs.rs (1)
14-32: 🧹 Nitpick | 🔵 TrivialUse
view.index()instead ofi + 1for consistency.Line 18 calculates the tab number as
i + 1, but ifViewhas anindex()method that returns the 1-based index, using it would ensure consistency if the view order or numbering ever changes.Proposed fix
let tabs: Vec<Span> = View::all() .iter() - .enumerate() - .flat_map(|(i, view)| { - let num = format!("{}", i + 1); + .flat_map(|view| { + let num = format!("{}", view.index()); let label = view.label();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tui/src/ui/tabs.rs` around lines 14 - 32, Replace the manual index calculation i + 1 with the view-provided index to keep numbering consistent: when building the tab Spans in the iterator over View::all(), call view.index() (instead of using the enumerated i) to produce the tab number string used in format!("{num}:{label}") — keep using view.label() for the label and the existing style/active comparison with active to preserve behavior.tui/src/ui/models.rs (1)
18-18:⚠️ Potential issue | 🟡 MinorAlign the column header with rendered data.
Line 18 says
OWNER, but Line 51 rendersm.display_name. Rename the header (or render owner) so the column meaning is accurate.Minimal fix
- let header = Row::new(vec!["ID", "OWNER", "WORKERS", "CREATED"]).style( + let header = Row::new(vec!["ID", "DISPLAY NAME", "WORKERS", "CREATED"]).style(Also applies to: 49-52
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tui/src/ui/models.rs` at line 18, The header Row::new currently labels the column "OWNER" but the rendered cell uses m.display_name, causing a mismatch; update the header text in the Row::new call (the header variable) to match the rendered field (e.g., "DISPLAY NAME" or "NAME"), or alternatively change the rendering where m.display_name is used to render an actual owner field; locate the Row::new(...) that defines header and the rendering loop that uses m.display_name and make the header and rendered field names consistent.tui/README.md (2)
66-70:⚠️ Potential issue | 🟡 MinorAdd language identifiers to fenced code blocks.
Lines 66, 168, and 223 use unlabeled fenced blocks and trigger MD040. Use
textfor these examples.Proposed fix
-``` +```text WORKERS CIRCUIT BREAKERS REQ/S AVG LATENCY 5 5 250.3 450ms all healthy all closed 51 in-flight ▓▓▓░░░░░@@
-+text
a:TUI b:SMG c:Llama-37595 c:Qwen2-39223 j/k scroll G bottom c cycle@@ -``` +```text :add <url> [--provider <p>] [--runtime <r>] :delete <id> :priority <number> :cost <number> :flush-cache :toggle-health :quit</details> Also applies to: 168-170, 223-231 <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@tui/README.mdaround lines 66 - 70, The three unlabeled fenced code blocks
in tui/README.md (the ASCII status table starting with "WORKERS CIRCUIT
BREAKERS", the key legend line starting with "a:TUI b:SMG c:Llama-37595", and
the command list starting with ":add ") are triggering MD040; add the
language identifier text to each opening triple-backtick fence (i.e., changetotext for the blocks containing those exact snippets) so the fenced blocks
are properly labeled.</details> --- `148-149`: _⚠️ Potential issue_ | _🟡 Minor_ **Use one canonical chat endpoint across the README.** Line 148 documents `/v1/chat`, while Line 273 documents `/v1/chat/completions`. Keep these consistent to avoid copy/paste misconfiguration. Also applies to: 273-273 <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@tui/README.md` around lines 148 - 149, Choose a single canonical chat endpoint and make all README occurrences consistent: replace `/v1/chat/completions` with `/v1/chat` (or vice versa) throughout the file, and update any example request/response blocks, headings, and references that mention `/v1/chat` or `/v1/chat/completions` so they match; also ensure the section that contrasts chat with the Responses API still references `previous_response_id` and `/v1/responses` correctly. ``` </details> </blockquote></details> <details> <summary>tui/src/ui/workers.rs (2)</summary><blockquote> `269-283`: _⚠️ Potential issue_ | _🟠 Major_ **Use one clamped selection for both table highlight and detail pane.** Line 272 clamps selection for the table, but Line 281 still indexes with raw `app.selected_index`. After filtering/deletion, highlighted row and detail panel can diverge. <details> <summary>Proposed fix</summary> ```diff - let mut table_state = TableState::default(); - if row_count > 0 { - table_state.select(Some(app.selected_index.min(row_count.saturating_sub(1)))); - } + let selected = if row_count > 0 { + Some(app.selected_index.min(row_count.saturating_sub(1))) + } else { + None + }; + let mut table_state = TableState::default(); + table_state.select(selected); @@ - if let Some(detail_area) = detail_area { - if let Some(worker) = filtered.get(app.selected_index) { + if let Some(detail_area) = detail_area { + if let Some(worker) = selected.and_then(|idx| filtered.get(idx)) { detail::render_detail(f, app, worker, detail_area); } } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@tui/src/ui/workers.rs` around lines 269 - 283, Compute a single clamped selection index and use it for both the table selection and detail lookup instead of using app.selected_index twice; e.g., create a local let selected = if row_count > 0 { app.selected_index.min(row_count.saturating_sub(1)) } else { 0 }; call table_state.select(Some(selected)) and then use filtered.get(selected) when calling detail::render_detail so the highlighted row and detail pane always refer to the same clamped index. ``` </details> --- `321-347`: _⚠️ Potential issue_ | _🟠 Major_ **Aggregate all load shards and broaden RPS fallback.** Line 329 only reads `details.loads.first()`, which under-reports TP>1 workers. Also, Line 337/342 fallback to `worker_rps` only for `grpc://` and `https://`; plain `http://` workers can incorrectly show `0`. </blockquote></details> <details> <summary>tui/src/ui/dialog.rs (2)</summary><blockquote> `40-43`: _🧹 Nitpick_ | _🔵 Trivial_ **Replace tuple indexing with destructuring for dialog text.** Line 42 and Line 83 use `info.0/info.1`, which is harder to read in formatted prompts. Destructure once into named locals before `format!`. <details> <summary>Readability improvement</summary> ```diff - let text = format!( + let (id, url) = info; + let text = format!( "Delete worker?\n\nID: {}\nURL: {}\n\n[y] confirm [n/Esc] cancel", - info.0, info.1, + id, url, ); ``` </details> Also applies to: 81-84 <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@tui/src/ui/dialog.rs` around lines 40 - 43, Destructure the tuple `info` into named locals (e.g., `let (id, url) = info;`) before building the dialog text and anywhere else `info.0` / `info.1` are used (the `format!` call that produces the "Delete worker?" text and the other dialog usage later), then replace `info.0` and `info.1` with the new `id` and `url` identifiers to improve readability in functions handling the dialog text (where `info` is referenced). ``` </details> --- `19-30`: _🧹 Nitpick_ | _🔵 Trivial_ **Extract shared popup-rect logic to a helper.** Line 19 and Line 60 duplicate the same centering/layout code. A small helper will keep both dialogs consistent and reduce drift when popup sizing changes. <details> <summary>Refactor sketch</summary> ```diff +fn centered_popup(area: ratatui::layout::Rect, width: u16, height: u16) -> ratatui::layout::Rect { + let [_, vert, _] = Layout::vertical([ + Constraint::Fill(1), + Constraint::Length(height), + Constraint::Fill(1), + ]).areas(area); + let [popup] = Layout::horizontal([Constraint::Length(width)]) + .flex(Flex::Center) + .areas(vert); + popup +} @@ - let area = f.area(); - let [_, vert, _] = Layout::vertical([...]).areas(area); - let [popup] = Layout::horizontal([Constraint::Length(50)]).flex(Flex::Center).areas(vert); + let popup = centered_popup(f.area(), 50, 8); ``` </details> Also applies to: 60-71 <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@tui/src/ui/dialog.rs` around lines 19 - 30, Extract the duplicated centering/layout code into a small helper (e.g., fn centered_popup(area: Rect, width: u16, height: u16) -> Rect) that performs the Layout::vertical/Horizontal sequence using Constraint::Length and Flex::Center and returns the resulting popup rect; replace both occurrences that currently bind [_, vert, _] and [popup] with a call to centered_popup(area, 50, 8) (or the appropriate width/height) so the dialog drawing code uses the single helper instead of duplicating the Layout logic. Ensure the helper references Layout, Constraint, and Flex and accepts the input `area` so both existing call sites (the blocks creating `vert`/`popup`) can be swapped to the new function. ``` </details> </blockquote></details> <details> <summary>tui/src/ui/help.rs (1)</summary><blockquote> `40-48`: _⚠️ Potential issue_ | _🟡 Minor_ **Rename the second “Navigation” section to avoid ambiguity.** Line 43 repeats the same section header used at Line 14, but this block is specifically for list movement. Rename it to “Selection” (or similar) so users can distinguish view switching from item navigation. <details> <summary>Proposed wording change</summary> ```diff -Navigation +Selection j / Down Move selection down k / Up Move selection up ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@tui/src/ui/help.rs` around lines 40 - 48, Update the duplicate section header inside the string passed to text.push_str so the second "Navigation" block is renamed to "Selection" (or similar) to clarify it refers to list/item movement rather than view switching; locate the string literal in tui/src/ui/help.rs where text.push_str(...) is called and change the header line "Navigation" to "Selection" while leaving the rest of the key descriptions unchanged. ``` </details> </blockquote></details> <details> <summary>tui/src/ui/logs.rs (1)</summary><blockquote> `163-207`: _⚠️ Potential issue_ | _🟠 Major_ **Move log file reading out of the render path.** Line 164 performs blocking disk I/O, and Lines 187-190 walk the full file on each frame. This will cause visible UI stalls as logs grow. Render should read from an already-cached tail maintained by background polling. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@tui/src/ui/logs.rs` around lines 163 - 207, render_file_log currently does blocking disk I/O and recomputes the file tail every frame; move file reading and line processing into a background poller that maintains a cached tail on the App and make render_file_log read only that cached data. Implement a background task (spawned at app init) that reads the log file path, trims to max_lines (500), strips ANSI and maps to styled Line objects once, and stores the result in a thread-safe field on App (e.g., Arc<RwLock<Vec<Line>>> or similar); update the poller at an interval (e.g., 250–500ms) and handle file-not-found there. Change render_file_log to fetch the precomputed Vec<Line> from App and render it without doing any file I/O or heavy processing (leave strip_ansi only in the poller), and keep the existing formatting logic but applied only in the background updater so rendering is non-blocking. ``` </details> </blockquote></details> <details> <summary>tui/src/ui/footer.rs (1)</summary><blockquote> `25-37`: _⚠️ Potential issue_ | _🟡 Minor_ **Update the footer hint to advertise all seven views.** Lines 25 and 37 show `hint("1-5", "view")`, but this PR adds seven tabs (Pulse, Workers, Chat, Logs, Benchmark, Traffic, Mesh) and the README documents `1-7`. Users won't know `6` and `7` are available. <details> <summary>🩹 Proposed fix</summary> ```diff - hint("1-5", "view"), + hint("1-7", "view"), ... - hint("1-5", "view"), + hint("1-7", "view"), ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@tui/src/ui/footer.rs` around lines 25 - 37, The footer currently advertises only "1-5" views in the hint(...) calls; update both occurrences of hint("1-5", "view") in the footer generation (the branch bodies shown around the vec![...] blocks in tui::ui::footer.rs) to advertise "1-7" so the footer reflects the seven tabs (Pulse, Workers, Chat, Logs, Benchmark, Traffic, Mesh) added by this PR. ``` </details> </blockquote></details> <details> <summary>tui/src/ui/stats_bar.rs (1)</summary><blockquote> `78-85`: _⚠️ Potential issue_ | _🟡 Minor_ **Handle the zero-worker case explicitly.** When `total == 0` and `state.connected`, Lines 81-82 still evaluate `unhealthy == 0` as true and render `"all healthy"`, which is misleading for an empty gateway. <details> <summary>🩹 Proposed fix</summary> ```diff let health_text = if !state.connected { ("--".to_string(), theme::TEXT_MUTED) + } else if total == 0 { + ("no workers".to_string(), theme::TEXT_MUTED) } else if unhealthy == 0 { ("all healthy".to_string(), theme::GREEN) } else { (format!("{unhealthy} unhealthy"), theme::RED) }; ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@tui/src/ui/stats_bar.rs` around lines 78 - 85, The health-text logic incorrectly shows "all healthy" when total == 0; update the health_text assignment in stats_bar.rs to explicitly handle the zero-worker case before checking unhealthy: when state.connected is true and total == 0 return a clear indicator (e.g., "--" or "no workers", with theme::TEXT_MUTED) instead of "all healthy"; keep the existing branches for state.connected == false, unhealthy == 0, and unhealthy > 0, referencing the variables unhealthy, total, healthy and state.connected so the check order is total == 0 first. ``` </details> </blockquote></details> <details> <summary>tui/src/event.rs (1)</summary><blockquote> `46-53`: _⚠️ Potential issue_ | _🟠 Major_ **Filter to accept only `KeyEventKind::Press` key events.** The current code forwards all `Event::Key` events including `KeyEventKind::Repeat` and `KeyEventKind::Release` on terminals that emit them (notably Windows). This can cause actions to double-fire. <details> <summary>🩹 Proposed fix</summary> ```diff -use crossterm::event::{Event, EventStream, KeyEvent}; +use crossterm::event::{Event, EventStream, KeyEvent, KeyEventKind}; ... - Some(Ok(Event::Key(key))) - if tx.send(AppEvent::Key(key)).is_err() => { - break; - } + Some(Ok(Event::Key(key))) + if key.kind == KeyEventKind::Press => + { + if tx.send(AppEvent::Key(key)).is_err() { + break; + } + } + Some(Ok(Event::Key(_))) => {} ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@tui/src/event.rs` around lines 46 - 53, The Event::Key branch currently forwards all key events; change the match arm handling Some(Ok(Event::Key(key))) so it only sends AppEvent::Key when key.kind == KeyEventKind::Press (ignore KeyEventKind::Repeat and KeyEventKind::Release) — update the pattern/if guard around the Event::Key arm and ensure KeyEventKind is referenced/imported so tx.send(AppEvent::Key(key)) is only called for KeyEventKind::Press. ``` </details> </blockquote></details> <details> <summary>tui/src/ui/detail.rs (2)</summary><blockquote> `225-231`: _⚠️ Potential issue_ | _🔴 Critical_ **Fix byte-slicing vulnerability in `truncate_str` to prevent panics on non-ASCII worker IDs.** Line 229 uses `&s[..max.saturating_sub(1)]` which panics if the byte index falls on a non-UTF-8 character boundary. This crashes the TUI when truncating worker IDs containing multi-byte characters. <details> <summary>🩹 Proposed fix</summary> ```diff fn truncate_str(s: &str, max: usize) -> String { - if s.len() <= max { + if s.chars().count() <= max { s.to_string() } else { - format!("{}…", &s[..max.saturating_sub(1)]) + let prefix: String = s.chars().take(max.saturating_sub(1)).collect(); + format!("{prefix}…") } } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@tui/src/ui/detail.rs` around lines 225 - 231, The truncate_str function slices by byte index (&s[..max.saturating_sub(1)]) which can panic on multi-byte UTF-8 characters; change truncate_str to perform a character-aware truncate (e.g., iterate s.chars().take(...) or use char_indices to find a valid byte boundary) so you build a new String of at most max-1 characters and then append the ellipsis, ensuring you handle the case s.len() <= max by returning s.to_string(); update the function truncate_str accordingly to avoid direct byte-slicing. ``` </details> --- `130-143`: _⚠️ Potential issue_ | _🟠 Major_ **Don't derive circuit-breaker state from worker health.** Lines 131-136 map `worker.is_healthy` to `closed/open`, but these are different signals. A worker can remain healthy (passing health checks) while its circuit breaker is open (tripped due to errors). The detail panel will misreport the actual breaker state. Pull the breaker state from shared metrics/state instead of proxying through health. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@tui/src/ui/detail.rs` around lines 130 - 143, The circuit display currently derives cb_label and cb_color from worker.is_healthy which is incorrect; instead query the shared breaker state/store for this worker (e.g., call the circuit state accessor for the worker id or use worker.circuit_state / worker.metrics.circuit_open if available) and base cb_label ("open"/"closed") and cb_color (theme::RED/theme::GREEN) on that boolean. Update the code that builds the "Circuit:" Line (the Span/Style creation around cb_label/cb_color and the variables cb_label/cb_color) to use the true circuit boolean from the shared metrics/state rather than worker.is_healthy. Ensure the accessor you use is thread-safe and present in scope before replacing references. ``` </details> </blockquote></details> <details> <summary>tui/src/chat.rs (1)</summary><blockquote> `163-182`: _⚠️ Potential issue_ | _🟠 Major_ **Avoid duplicating the final response on `response.completed` / `response.done`.** If the stream already emitted `response.output_text.delta` chunks (Lines 163-167), Lines 168-182 append the full completed text again. This duplicates assistant output for backends that send both deltas and a final aggregated response. <details> <summary>🩹 Proposed fix</summary> ```diff let mut stream = resp.bytes_stream(); let mut buffer = String::new(); + let mut saw_text_delta = false; ... "response.output_text.delta" => { if let Some(delta) = parsed["delta"].as_str() { + saw_text_delta = true; let _ = tx.send(delta.to_string()); } } "response.completed" | "response.done" => { - // Extract text from completed response (sglang sends full text here, not deltas) - if let Some(outputs) = parsed["response"]["output"].as_array() { + // Only extract if no deltas were received (some backends send full text here) + if !saw_text_delta { + if let Some(outputs) = parsed["response"]["output"].as_array() { ... + } + } let _ = tx.send("\n[DONE]".to_string()); return; ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@tui/src/chat.rs` around lines 163 - 182, The completed/done branch duplicates assistant output when prior "response.output_text.delta" chunks were already sent; introduce a boolean flag (e.g., has_sent_deltas or saw_delta) in the surrounding scope, set it to true inside the "response.output_text.delta" arm where tx.send(delta.to_string()) is called, and in the "response.completed" | "response.done" arm skip sending the full aggregated text (and only send the final "[DONE]" marker) if that flag is true; ensure the flag is referenced where tx.send is used so only one representation of the assistant response is emitted. ``` </details> </blockquote></details> <details> <summary>tui/src/ui/action_menu.rs (1)</summary><blockquote> `145-167`: _⚠️ Potential issue_ | _🟠 Major_ **Remove the per-frame `Box::leak` allocations in SelectModel menu rendering.** Lines 151 and 155 leak numbering strings every frame the model picker is displayed. Since `render_add_menu` is called from the per-frame root renderer, keeping the SelectModel modal open causes unbounded heap growth. <details> <summary>🩹 Proposed fix</summary> Change `render_menu` to accept owned `String` items: ```diff -fn render_menu(f: &mut Frame, title: &str, items: &[(&str, &str, &str)]) { +fn render_menu(f: &mut Frame, title: &str, items: &[(String, String, String)]) { ... - Span::styled( - format!(" [{num}] "), + Span::styled( + format!(" [{}] ", num), ... - Span::styled(*label, Style::default().fg(theme::TEXT)), + Span::styled(label.as_str(), Style::default().fg(theme::TEXT)), // In SelectModel case: - let num = Box::leak(format!("{}", i + 1).into_boxed_str()) as &str; - (num, p.label(), format!("TP={}", p.tp())) + ((i + 1).to_string(), p.label(), format!("TP={}", p.tp())) ... - let custom_num = Box::leak(format!("{}", presets.len() + 1).into_boxed_str()) as &str; + let custom_num = (presets.len() + 1).to_string(); ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@tui/src/ui/action_menu.rs` around lines 145 - 167, The SelectModel branch is leaking numeric labels via Box::leak each frame; stop allocating leaked &'static str by switching to owned strings and updating the menu renderer. Build items as Vec<(String, String, String)> (create the numeric label with format! without Box::leak) in the AddMenuState::SelectModel arm, then change render_menu to accept owned String tuples (or a slice of (String,String,String)) instead of &str references and update any call sites accordingly so you can pass the owned items directly without leaking memory. ``` </details> </blockquote></details> <details> <summary>tui/src/ui/pulse.rs (1)</summary><blockquote> `21-38`: _⚠️ Potential issue_ | _🟠 Major_ **Keep request stats in the narrow layout.** This branch still returns after `render_throughput_compact()`, so small terminals never render latency, connections, or in-flight request stats. The compact vertical layout needs an extra row for `render_request_stats()`. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@tui/src/ui/pulse.rs` around lines 21 - 38, The narrow-layout branch returns too early and omits request stats; add another row to the constraints Vec (another Constraint::Fill(1)) when building the single-column layout and call render_request_stats(f, &state, rows[i]) in the sequence before the final render_throughput_compact (adjusting the index increments to account for the extra row and respecting has_node_panel), so Layout::vertical(...).split(area) produces an extra rows entry and render_request_stats is invoked for small terminals. ``` </details> </blockquote></details> <details> <summary>tui/src/main.rs (1)</summary><blockquote> `158-170`: _⚠️ Potential issue_ | _🟠 Major_ **Protect terminal setup with a guard, not only the panic hook.** Any early `?` after raw mode / alternate-screen setup but before normal teardown will exit without restoring the shell state. Keep a cleanup guard alive from the first successful terminal mutation and disarm it only after explicit teardown succeeds. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@tui/src/main.rs` around lines 158 - 170, The terminal setup currently relies only on a panic hook and can leak terminal state if an early `?` returns; create a RAII guard (e.g., a small struct like TerminalGuard with Drop that calls disable_raw_mode() and executes LeaveAlternateScreen) and instantiate it immediately after the first successful mutation (after enable_raw_mode() and EnterAlternateScreen execute!); keep a method on the guard to disarm or mark successful teardown and call that when you run the normal cleanup (before Terminal::draw/Terminal::new teardown completes); leave panic_hook/set_hook as-is but rely on the guard to ensure cleanup on any early returns or normal exit. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Inline comments:
In@tui/src/app.rs:
- Around line 603-607: The quit-on-
qbehavior in handle_chat_key currently
triggers whenever KeyCode::Char('q') and !self.chat_streaming, preventing typing
'q' into the chat; update the match arm so quitting only occurs when the input
buffer is empty (match the same empty-input gating used by other chat shortcuts)
— i.e., require both !self.chat_streaming and
self.<input_buffer_field>.is_empty() before setting self.should_quit = true;
modify the KeyCode::Char('q') branch accordingly so normal typing of 'q' in the
chat box is allowed.- Line 553: The unknown-command fallback is leaking typed secrets because the
"api-key " action isn't handled and falls through to the match arm that calls
set_status(...) which appends the full message into status_message and
log_entries; fix this by adding an explicit handler for the "api-key " command
(or by detecting the "api-key " prefix before the match) that accepts the
entered key but does not echo it back verbatim — instead call set_status with a
masked message like "API key updated" (or call a new method that updates state
without logging the secret), and ensure set_status is not given the raw secret;
reference the command parsing match in app.rs and the set_status, status_message
and log_entries symbols when implementing this change.- Around line 506-523: The "toggle-health" branch always sets
disable_health_check: Some(true) so it never re-enables checks; change it to
compute the next boolean from the selected worker's current state (e.g., via
self.selected_worker_id() -> lookup worker record/state and read
worker.health.disable_health_check) and pass the inverted value into
openai_protocol::worker::HealthCheckUpdate.disable_health_check before calling
self.client.update_worker(&id, &update). Update the same logic in the other
occurrence (lines ~876-900) or, if you intend a one-way action, rename the
action/label to "disable-health" to reflect that it only disables.- Around line 765-779: The selected worker resolution must use the same filtered
view as the UI; change the logic so all actions (start_delete,
priority/cost/flush/toggle handlers) map selected_index into the filtered worker
list rather than the unfiltered wl.workers. Implement a single helper (e.g.,
get_filtered_workers or refactor the filtering block) that returns the Vec of
filtered worker refs using the active_filter logic currently in start_delete,
and have selected_worker_id(), selected_worker_url(), and the action methods
call that helper and index into its result; ensure you handle out-of-range
selected_index safely (None) and replace any direct indexing into wl.workers
with lookups into the filtered list.- Around line 727-745: The current loop in state.workers processing (iterating
workers.workers and checking w.runtime_type and w.models) wrongly filters
external worker models to those starting with "gpt-5.4", dropping valid external
provider model IDs; remove the is_external && !m.id.starts_with("gpt-5.4") check
so you always collect each reported m.id into models (checking duplicates as
already done). Keep the wildcard handling for when w.models.is_empty(), but stop
hard-coding "gpt-5.4-nano" as the placeholder — instead use a provider-specific
default if the worker exposes one (e.g., a w.default_model or similar) or fall
back to a generic "external-model" placeholder so external providers
(Anthropic/xAI/Gemini/OpenAI variants) are selectable.- Around line 1217-1251: On registration failure in the add_worker() match arm,
undo the prior spawn/claim: remove the corresponding entry from
self.worker_children (the tuple containing desc and child), terminate or kill
the spawned child process (the child from the tuple) and await/clean up its
handle, and remove the claimed GPU entry from self.claimed_gpus keyed by url
(which was set from selected_gpus); keep the current error log and status
updates but ensure the worker process is stopped and the GPU claim released to
avoid phantom busy GPUs.- Around line 444-453: The :delete branch currently calls
self.client.delete_worker(...) directly, which only removes the worker from the
gateway and leaves local resources (spawned backend and claimed_gpus) orphaned;
change this branch to reuse the same cleanup/confirmation flow used by the
interactive delete dialog (the block later in the file that kills the spawned
backend and releases claimed_gpus) instead of calling self.client.delete_worker
directly — either call that existing confirmation/cleanup function or extract
the cleanup logic into a shared method (e.g., delete_worker_with_cleanup or
confirm_and_delete_worker) and invoke it from the Some("delete") arm, keeping
the same status updates via self.set_status.In
@tui/src/event.rs:
- Around line 31-32: Replace the comment prefix "Safety:" with "INVARIANT:" in
the inline doc above the fire-and-forget event reader loop comment (the comment
directly above the #[expect(clippy::disallowed_methods)] attribute in event.rs)
so it follows the repo convention of using INVARIANT: for assumptions in safe
code; keep the rest of the text unchanged and only rename the marker.In
@tui/src/main.rs:
- Around line 197-199: The current extract_port function misparses URLs with
paths because it splits on ':' and grabs the trailing segment; replace this
brittle string logic with proper URL parsing: use
url::Url::parse(url).ok().and_then(|u| u.port()) (i.e., call Url::parse in
extract_port and return u.port()), so inputs like
"http://localhost:31000/health" return 31000; add the url crate if it's not
already a dependency and update extract_port to use Url parsing instead of
rsplit.In
@tui/src/ui/chat.rs:
- Around line 154-164: The current wrapped-line calculation uses byte counts
(span.content.len()) which miscounts Unicode/emoji/CJK; replace that with
display width using unicode-width's UnicodeWidthStr::width (add the
unicode-width crate and import UnicodeWidthStr), i.e. compute each span's
display width via span.content.width(), sum those to get line_width, then
perform the same ceil division by width to produce the u16 contribution to
total_lines (refer to variables/expressions: total_lines, lines, line.spans,
span.content, and width).In
@tui/src/ui/detail.rs:
- Around line 32-34: Change the comment marker from "Safety:" to "INVARIANT:"
for the non-unsafe RwLock read assumption: update the comment above the call to
app.state.read().unwrap() to begin with "INVARIANT:" and retain the explanation
that the RwLock is not poisoned (and keep the #[expect(clippy::unwrap_used)]
attribute) so the repository convention of reserving SAFETY: for unsafe blocks
is followed.In
@tui/src/ui/pulse.rs:
- Around line 371-389: In render_throughput_compact the UI reads latest from
throughput_history but labels it as req/s and also uses throughput_history
emptiness to decide "No data"; change the logic to use requests_per_sec_history
instead: check requests_per_sec_history.is_empty() for the "No data" early
return and compute latest from
requests_per_sec_history.back().copied().unwrap_or(0.0), keeping the displayed
label "Latest: {latest:.1} req/s"; leave throughput_history usage elsewhere
unchanged.In
@tui/src/ui/sparkline.rs:
- Around line 46-50: Clamp the input ratio in gauge_bar to the [0.0, 1.0] range
before computing pct and filled so the displayed percentage (pct) and bar fill
stay consistent; inside pub fn gauge_bar(ratio: f64, width: usize) create a
local let r = ratio.clamp(0.0, 1.0) (or equivalent), then compute pct from r and
compute filled using r and width, leaving empty and return values unchanged.In
@tui/src/ui/workers.rs:
- Around line 299-304: The truncate() function currently slices by byte index
(&s[..max - 1]) which can panic on multi-byte UTF-8 characters (like emoji) —
replace byte-slicing with character-based truncation: when s.len() > max,
collect the first (max - 1) Unicode scalar values (e.g., s.chars().take(max -
1).collect::()) and append the ellipsis; keep the original behavior for
short strings. Update the truncate function (used for worker.id) to avoid any
direct byte-index slicing and operate on chars to ensure UTF-8 safety.
Duplicate comments:
In@tui/README.md:
- Around line 66-70: The three unlabeled fenced code blocks in tui/README.md
(the ASCII status table starting with "WORKERS CIRCUIT BREAKERS", the
key legend line starting with "a:TUI b:SMG c:Llama-37595", and the command
list starting with ":add ") are triggering MD040; add the language
identifier text to each opening triple-backtick fence (i.e., change ``` toproperly labeled. - Around line 148-149: Choose a single canonical chat endpoint and make all README occurrences consistent: replace `/v1/chat/completions` with `/v1/chat` (or vice versa) throughout the file, and update any example request/response blocks, headings, and references that mention `/v1/chat` or `/v1/chat/completions` so they match; also ensure the section that contrasts chat with the Responses API still references `previous_response_id` and `/v1/responses` correctly. In `@tui/src/chat.rs`: - Around line 163-182: The completed/done branch duplicates assistant output when prior "response.output_text.delta" chunks were already sent; introduce a boolean flag (e.g., has_sent_deltas or saw_delta) in the surrounding scope, set it to true inside the "response.output_text.delta" arm where tx.send(delta.to_string()) is called, and in the "response.completed" | "response.done" arm skip sending the full aggregated text (and only send the final "[DONE]" marker) if that flag is true; ensure the flag is referenced where tx.send is used so only one representation of the assistant response is emitted. In `@tui/src/client.rs`: - Around line 264-266: The streaming code builds a new reqwest::Client inside stream_request which prevents connection pooling and TLS session reuse; instead add a dedicated streaming client field (e.g., stream_client: reqwest::Client) to the client struct initialized in the constructor using Client::builder().timeout(Duration::from_secs(120)). Replace the local builder call in stream_request with a reuse of self.stream_client so all streaming requests reuse the same client and timeout configuration. In `@tui/src/event.rs`: - Around line 46-53: The Event::Key branch currently forwards all key events; change the match arm handling Some(Ok(Event::Key(key))) so it only sends AppEvent::Key when key.kind == KeyEventKind::Press (ignore KeyEventKind::Repeat and KeyEventKind::Release) — update the pattern/if guard around the Event::Key arm and ensure KeyEventKind is referenced/imported so tx.send(AppEvent::Key(key)) is only called for KeyEventKind::Press. In `@tui/src/main.rs`: - Around line 158-170: The terminal setup currently relies only on a panic hook and can leak terminal state if an early `?` returns; create a RAII guard (e.g., a small struct like TerminalGuard with Drop that calls disable_raw_mode() and executes LeaveAlternateScreen) and instantiate it immediately after the first successful mutation (after enable_raw_mode() and EnterAlternateScreen execute!); keep a method on the guard to disarm or mark successful teardown and call that when you run the normal cleanup (before Terminal::draw/Terminal::new teardown completes); leave panic_hook/set_hook as-is but rely on the guard to ensure cleanup on any early returns or normal exit. In `@tui/src/state.rs`: - Around line 395-398: throughput_history is being appended twice per poll cycle because total_throughput is pushed unconditionally and later rps (from Prometheus) can also push; update the logic so the sparkline gets a single source per cycle: prefer rps when available by making the first push conditional (e.g., only push total_throughput into s.throughput_history when rps is not available) or convert the two pushes into mutually exclusive branches; touch the block that uses SPARKLINE_CAP and total_throughput as well as the later branch that pushes rps to ensure only one push to s.throughput_history happens per cycle. - Around line 95-105: spawn_poller currently drops the tokio::task::JoinHandle which prevents graceful shutdown; change spawn_poller to return the JoinHandle (tokio::task::JoinHandle<()>) instead of () by capturing the result of tokio::spawn and returning it, update callers to hold and abort/await the handle during shutdown, and ensure the signature in state.rs (spawn_poller) and any call sites are updated to propagate or store the returned JoinHandle so the poller can be cancelled or awaited cleanly. - Around line 451-466: The subtraction cur_sum - prev_sum in the parse_duration_stats handling can produce negative delta_sum when Prometheus resets counters; update the logic in the block around parse_duration_stats / s.prev_duration_sum / s.prev_duration_count so you compute delta_sum = (cur_sum as f64 - prev_sum as f64) and then guard: if delta_sum > 0.0 && delta_count > 0 { avg_latency = delta_sum / delta_count as f64; push to s.avg_latency_history (evict oldest when >= SPARKLINE_CAP) } else skip pushing (or treat avg_latency as 0.0 but do not push negative values). Ensure prev_duration_sum and prev_duration_count are still updated after the check. In `@tui/src/ui/action_menu.rs`: - Around line 145-167: The SelectModel branch is leaking numeric labels via Box::leak each frame; stop allocating leaked &'static str by switching to owned strings and updating the menu renderer. Build items as Vec<(String, String, String)> (create the numeric label with format! without Box::leak) in the AddMenuState::SelectModel arm, then change render_menu to accept owned String tuples (or a slice of (String,String,String)) instead of &str references and update any call sites accordingly so you can pass the owned items directly without leaking memory. In `@tui/src/ui/detail.rs`: - Around line 225-231: The truncate_str function slices by byte index (&s[..max.saturating_sub(1)]) which can panic on multi-byte UTF-8 characters; change truncate_str to perform a character-aware truncate (e.g., iterate s.chars().take(...) or use char_indices to find a valid byte boundary) so you build a new String of at most max-1 characters and then append the ellipsis, ensuring you handle the case s.len() <= max by returning s.to_string(); update the function truncate_str accordingly to avoid direct byte-slicing. - Around line 130-143: The circuit display currently derives cb_label and cb_color from worker.is_healthy which is incorrect; instead query the shared breaker state/store for this worker (e.g., call the circuit state accessor for the worker id or use worker.circuit_state / worker.metrics.circuit_open if available) and base cb_label ("open"/"closed") and cb_color (theme::RED/theme::GREEN) on that boolean. Update the code that builds the "Circuit:" Line (the Span/Style creation around cb_label/cb_color and the variables cb_label/cb_color) to use the true circuit boolean from the shared metrics/state rather than worker.is_healthy. Ensure the accessor you use is thread-safe and present in scope before replacing references. In `@tui/src/ui/dialog.rs`: - Around line 40-43: Destructure the tuple `info` into named locals (e.g., `let (id, url) = info;`) before building the dialog text and anywhere else `info.0` / `info.1` are used (the `format!` call that produces the "Delete worker?" text and the other dialog usage later), then replace `info.0` and `info.1` with the new `id` and `url` identifiers to improve readability in functions handling the dialog text (where `info` is referenced). - Around line 19-30: Extract the duplicated centering/layout code into a small helper (e.g., fn centered_popup(area: Rect, width: u16, height: u16) -> Rect) that performs the Layout::vertical/Horizontal sequence using Constraint::Length and Flex::Center and returns the resulting popup rect; replace both occurrences that currently bind [_, vert, _] and [popup] with a call to centered_popup(area, 50, 8) (or the appropriate width/height) so the dialog drawing code uses the single helper instead of duplicating the Layout logic. Ensure the helper references Layout, Constraint, and Flex and accepts the input `area` so both existing call sites (the blocks creating `vert`/`popup`) can be swapped to the new function. In `@tui/src/ui/footer.rs`: - Around line 25-37: The footer currently advertises only "1-5" views in the hint(...) calls; update both occurrences of hint("1-5", "view") in the footer generation (the branch bodies shown around the vec![...] blocks in tui::ui::footer.rs) to advertise "1-7" so the footer reflects the seven tabs (Pulse, Workers, Chat, Logs, Benchmark, Traffic, Mesh) added by this PR. In `@tui/src/ui/help.rs`: - Around line 40-48: Update the duplicate section header inside the string passed to text.push_str so the second "Navigation" block is renamed to "Selection" (or similar) to clarify it refers to list/item movement rather than view switching; locate the string literal in tui/src/ui/help.rs where text.push_str(...) is called and change the header line "Navigation" to "Selection" while leaving the rest of the key descriptions unchanged. In `@tui/src/ui/logs.rs`: - Around line 163-207: render_file_log currently does blocking disk I/O and recomputes the file tail every frame; move file reading and line processing into a background poller that maintains a cached tail on the App and make render_file_log read only that cached data. Implement a background task (spawned at app init) that reads the log file path, trims to max_lines (500), strips ANSI and maps to styled Line objects once, and stores the result in a thread-safe field on App (e.g., Arc<RwLock<Vec<Line>>> or similar); update the poller at an interval (e.g., 250–500ms) and handle file-not-found there. Change render_file_log to fetch the precomputed Vec<Line> from App and render it without doing any file I/O or heavy processing (leave strip_ansi only in the poller), and keep the existing formatting logic but applied only in the background updater so rendering is non-blocking. In `@tui/src/ui/models.rs`: - Line 18: The header Row::new currently labels the column "OWNER" but the rendered cell uses m.display_name, causing a mismatch; update the header text in the Row::new call (the header variable) to match the rendered field (e.g., "DISPLAY NAME" or "NAME"), or alternatively change the rendering where m.display_name is used to render an actual owner field; locate the Row::new(...) that defines header and the rendering loop that uses m.display_name and make the header and rendered field names consistent. In `@tui/src/ui/pulse.rs`: - Around line 21-38: The narrow-layout branch returns too early and omits request stats; add another row to the constraints Vec (another Constraint::Fill(1)) when building the single-column layout and call render_request_stats(f, &state, rows[i]) in the sequence before the final render_throughput_compact (adjusting the index increments to account for the extra row and respecting has_node_panel), so Layout::vertical(...).split(area) produces an extra rows entry and render_request_stats is invoked for small terminals. In `@tui/src/ui/stats_bar.rs`: - Around line 78-85: The health-text logic incorrectly shows "all healthy" when total == 0; update the health_text assignment in stats_bar.rs to explicitly handle the zero-worker case before checking unhealthy: when state.connected is true and total == 0 return a clear indicator (e.g., "--" or "no workers", with theme::TEXT_MUTED) instead of "all healthy"; keep the existing branches for state.connected == false, unhealthy == 0, and unhealthy > 0, referencing the variables unhealthy, total, healthy and state.connected so the check order is total == 0 first. In `@tui/src/ui/tabs.rs`: - Around line 14-32: Replace the manual index calculation i + 1 with the view-provided index to keep numbering consistent: when building the tab Spans in the iterator over View::all(), call view.index() (instead of using the enumerated i) to produce the tab number string used in format!("{num}:{label}") — keep using view.label() for the label and the existing style/active comparison with active to preserve behavior. In `@tui/src/ui/workers.rs`: - Around line 269-283: Compute a single clamped selection index and use it for both the table selection and detail lookup instead of using app.selected_index twice; e.g., create a local let selected = if row_count > 0 { app.selected_index.min(row_count.saturating_sub(1)) } else { 0 }; call table_state.select(Some(selected)) and then use filtered.get(selected) when calling detail::render_detail so the highlighted row and detail pane always refer to the same clamped index.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID:
0fdd8da1-311b-4f9f-9662-e2c4b7ae6f72⛔ Files ignored due to path filters (2)
tui/assets/add-worker.gifis excluded by!**/*.giftui/assets/tui-demo.gifis excluded by!**/*.gif📒 Files selected for processing (28)
.pre-commit-config.yamlCargo.tomltui/Cargo.tomltui/README.mdtui/src/app.rstui/src/chat.rstui/src/client.rstui/src/event.rstui/src/lib.rstui/src/main.rstui/src/state.rstui/src/types.rstui/src/ui/action_menu.rstui/src/ui/chat.rstui/src/ui/detail.rstui/src/ui/dialog.rstui/src/ui/filter.rstui/src/ui/footer.rstui/src/ui/help.rstui/src/ui/logs.rstui/src/ui/mod.rstui/src/ui/models.rstui/src/ui/pulse.rstui/src/ui/sparkline.rstui/src/ui/stats_bar.rstui/src/ui/tabs.rstui/src/ui/theme.rstui/src/ui/workers.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 95a1ee0fe9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (27)
tui/src/event.rs (2)
31-32: 🧹 Nitpick | 🔵 TrivialUse
INVARIANT:instead ofSafety:here.This is documenting a safe-code assumption, not an unsafe soundness requirement.
Based on learnings: "In Rust code across the repository, use the marker INVARIANT: to document assumptions in safe code. Reserve SAFETY: for explaining why unsafe blocks are sound."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tui/src/event.rs` around lines 31 - 32, Change the comment prefix from "Safety:" to "INVARIANT:" for the safe-code assumption in the event reader description so it follows repository convention; update the comment that currently reads "// Safety: fire-and-forget event reader loop that runs for the app's lifetime" to use "INVARIANT:" instead (this is right above the #[expect(clippy::disallowed_methods)] attribute in event.rs and refers to the event reader loop assumption).
1-1:⚠️ Potential issue | 🟠 MajorOnly enqueue
KeyEventKind::Pressevents.Forwarding every
Event::Keywill double-fire actions on terminals that emitRepeat/Release. Filter here so the rest of the app only sees key presses.🩹 Proposed fix
-use crossterm::event::{Event, EventStream, KeyEvent}; +use crossterm::event::{Event, EventStream, KeyEvent, KeyEventKind}; ... - Some(Ok(Event::Key(key))) - if tx.send(AppEvent::Key(key)).is_err() => { - break; - } + Some(Ok(Event::Key(key))) if key.kind == KeyEventKind::Press => { + if tx.send(AppEvent::Key(key)).is_err() { + break; + } + } + Some(Ok(Event::Key(_))) => {}In crossterm 0.28.x, can Event::Key emit KeyEventKind::Repeat/Release, and do Ratatui examples recommend handling only KeyEventKind::Press to avoid duplicate key processing?Also applies to: 44-55
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tui/src/event.rs` at line 1, The event forwarder is emitting all crossterm Event::Key variants causing duplicate actions on Repeat/Release; update the code that matches Event::Key (in tui/src/event.rs) to only enqueue/forward when the contained KeyEvent has kind == KeyEventKind::Press (i.e., filter Event::Key(k) by k.kind == KeyEventKind::Press) so the rest of the app receives only key presses.tui/src/ui/workers.rs (3)
299-304:⚠️ Potential issue | 🔴 CriticalMake
truncate()UTF-8 safe.This currently treats
maxas a byte limit, not a character limit, so non-ASCII IDs/URLs/model lists can truncate too early or panic when&s[..max - 1]lands inside a codepoint.🩹 Proposed fix
fn truncate(s: &str, max: usize) -> String { - if s.len() <= max { + if s.chars().count() <= max { s.to_string() } else { - format!("{}…", &s[..max - 1]) + let head: String = s.chars().take(max.saturating_sub(1)).collect(); + format!("{head}…") } }In Rust, can slicing a &str with byte indices like &s[..n] panic when n is not on a UTF-8 character boundary?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tui/src/ui/workers.rs` around lines 299 - 304, The truncate(s: &str, max: usize) function currently slices by bytes which can panic on UTF-8 boundaries; change it to treat max as a character limit by operating on chars rather than byte indices (e.g., iterate s.chars() and collect up to max characters, appending the ellipsis when the original string has more than max chars). Update truncate to check character count, return s.to_string() when length <= max, otherwise build a new String from the first max-1 characters (or first max and then replace last with ellipsis per existing behavior) using char iteration so all slicing is UTF-8 safe; reference function name truncate in this file.
269-283:⚠️ Potential issue | 🟠 MajorReuse the clamped selection for the detail pane.
The table highlight clamps
selected_index, but Lines 280-282 still use the raw index. After filtering or worker deletion, the highlighted row can differ from the detail pane or leave it blank.🩹 Proposed fix
- let mut table_state = TableState::default(); - if row_count > 0 { - table_state.select(Some(app.selected_index.min(row_count.saturating_sub(1)))); - } + let selected = if row_count > 0 { + Some(app.selected_index.min(row_count.saturating_sub(1))) + } else { + None + }; + + let mut table_state = TableState::default(); + table_state.select(selected); @@ - if let Some(detail_area) = detail_area { - if let Some(worker) = filtered.get(app.selected_index) { + if let Some(detail_area) = detail_area { + if let Some(worker) = selected.and_then(|idx| filtered.get(idx)) { detail::render_detail(f, app, worker, detail_area); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tui/src/ui/workers.rs` around lines 269 - 283, The detail pane currently uses the raw app.selected_index which can be out-of-sync with the clamped selection used for the table; after you call table_state.select(...) read the clamped index from table_state.selected() and use that to look up the worker for detail::render_detail instead of app.selected_index. Concretely, after building table_state use something like if let Some(clamped) = table_state.selected() { if let Some(worker) = filtered.get(clamped) { detail::render_detail(f, app, worker, detail_area); } } so the detail pane always follows the table's actual highlighted row.
326-346:⚠️ Potential issue | 🟠 MajorDon't drop extra load entries or plain-HTTP fallbacks.
details.loads.first()under-reports TP>1 workers, and the fallback only coversgrpc:///https://. Plainhttp://workers fall through to"0"whenever/get_loadsis unavailable even thoughworker_rpsis already available.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tui/src/ui/workers.rs` around lines 326 - 346, The current code drops extra load entries by using details.loads.first() and also fails to use worker_rps for plain "http://" fallbacks; change the logic in the worker-load formatting block to (1) aggregate across all entries in details.loads (e.g., total_running = sum(load.num_running_reqs) and token_usage = max(load.token_usage) or a sensible aggregate) instead of details.loads.first(), and (2) move/extend the rps-fallback to cover plain "http://" (or, more generally, if worker_rps contains an entry use that) so that when /get_loads is missing you return (format!("{rps:.1} r/s"), "N/A".to_string()) rather than ("0", "0.0%"). Reference symbols: loads.get(worker_url), wl.details, details.loads, worker_url.starts_with(...), and worker_rps.get(worker_url).tui/src/ui/tabs.rs (1)
14-18: 🧹 Nitpick | 🔵 TrivialUse
View::index()as the tab number source.
enumerate()+i + 1duplicates numbering logic thatViewalready owns, so the rendered shortcut can drift if the enum numbering changes.view.index()keeps the tab label aligned with the rest of the navigation code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tui/src/ui/tabs.rs` around lines 14 - 18, The tab numbering currently uses enumerate() and i + 1 when building tabs from View::all(), which duplicates View's own numbering logic; update the closure that constructs the tab label (the block producing num = format!("{}", i + 1)) to use view.index() as the source of the tab number instead (call view.index() and format that value), removing reliance on enumerate() for numbering so the rendered shortcut stays consistent with View::index() across the codebase.tui/src/ui/footer.rs (1)
25-25:⚠️ Potential issue | 🟡 MinorAdvertise all seven views in the footer.
Both normal-mode hint sets still say
1-5, but the tab bar exposes seven views. That makesTrafficandMeshlook unavailable from the keyboard.🩹 Proposed fix
- hint("1-5", "view"), + hint("1-7", "view"), ... - hint("1-5", "view"), + hint("1-7", "view"),Also applies to: 37-37
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tui/src/ui/footer.rs` at line 25, The footer currently advertises only "1-5" in the normal-mode hint sets, but the UI exposes seven views; update the two hint label calls so the footer advertises all seven views. Locate the hint("1-5", "view") invocations in tui/src/ui/footer.rs (the two occurrences around the normal-mode hint blocks) and change the displayed range to "1-7" (e.g., hint("1-7", "view")) so both normal-mode hint sets advertise all seven views including Traffic and Mesh.tui/src/ui/models.rs (1)
18-18:⚠️ Potential issue | 🟡 MinorFix the
OWNERcolumn label or the rendered value.Line 18 says
OWNER, but Line 51 rendersm.display_name. That makes the table misleading as soon as display name differs from the owner. Rename the header or render the actual owner field.Also applies to: 49-51
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tui/src/ui/models.rs` at line 18, Header text "OWNER" in the table (created via Row::new in the header variable) does not match the rendered cell value which uses m.display_name; change one to match the other by either renaming the header to "NAME" (or "OWNER / NAME") where header is built in Row::new, or update the cell renderer to display the actual owner field (e.g., replace m.display_name with m.owner or m.owner_id) in the code that builds the row cells around where m is used (lines rendering the row, e.g., the code around m.display_name). Ensure the header label and the value source are consistent across the table rendering.tui/src/ui/logs.rs (1)
163-190:⚠️ Potential issue | 🟠 MajorMove log-file reads and tailing out of
render_file_log().
read_to_string()pluscontent.lines().collect()runs on every frame, so a large or fast-growing log will stall repaint/input exactly when the Logs view is open. Populate a cached tail in the background poller and render only that cached slice here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tui/src/ui/logs.rs` around lines 163 - 190, render_file_log currently reads and parses the entire file on every frame (via std::fs::read_to_string and content.lines().collect), which blocks the UI; move all file I/O and tailing into the background poller and have render_file_log only render a cached tail. Specifically: add a cached tail field (e.g., App.cached_log_tail: Vec<Line> or String) updated by the poller, change the poller to read the file, compute the last N lines (max_lines=500), and store the result into that cache using a thread-safe mechanism (Arc<Mutex<...>> or a channel update on App), and modify render_file_log to skip read_to_string and instead render from App.cached_log_tail (still using the same Block/Paragraph rendering and the existing title/label logic).tui/src/ui/chat.rs (2)
22-37:⚠️ Potential issue | 🟡 MinorShow the real chat-completions path in the title.
For
ChatEndpoint::Chat, this renders/v1/chat, buttui/src/chat.rsactually posts to/v1/chat/completions. The title becomes misleading when users compare the TUI against logs/docs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tui/src/ui/chat.rs` around lines 22 - 37, The title shows an incorrect path for ChatEndpoint::Chat; update the title formatting in chat UI to display the actual POST path used by the client ("/v1/chat/completions") instead of "/v1/chat" when app.chat_endpoint is Chat; locate the code that builds the title (uses app.chat_endpoint, ChatEndpoint::Chat and app.chat_endpoint.label()) and either change the label() call for Chat to return "chat/completions" or special-case Chat in this formatting to insert "/v1/chat/completions" so the displayed path matches the request path used by tui/src/chat.rs.
152-164:⚠️ Potential issue | 🟡 MinorUse display width, not byte length, for scroll math.
span.content.len()counts bytes, so emoji/CJK text skewstotal_linesand makes the wrapped scroll range drift from what Ratatui actually renders.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tui/src/ui/chat.rs` around lines 152 - 164, The scroll math is using byte length via span.content.len(), which miscalculates display width for emoji/CJK; change the width calculation in the total_lines mapping to use a display-width function (e.g. UnicodeWidthStr::width) on each span's content instead of len(), keep the max(1) and ceil division logic, and add the necessary import for the unicode-width helper so total_lines reflects visual columns rather than byte count.tui/src/ui/stats_bar.rs (1)
79-85:⚠️ Potential issue | 🟡 MinorHandle the zero-worker case explicitly.
When the gateway is connected but
total == 0, this still renders greenall healthy. That should be a neutralno workers/--state instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tui/src/ui/stats_bar.rs` around lines 79 - 85, The health_text branch logic treats a connected gateway with total == 0 as "all healthy"; change health_text (used where state.connected, unhealthy, total are referenced) so that when state.connected is true and total == 0 it returns the neutral/no-workers variant (e.g., "--" or "no workers" with theme::TEXT_MUTED) before checking unhealthy == 0; reorder the conditions accordingly so the total==0 case is handled first while preserving the existing GREEN for actual healthy workers and RED for unhealthy cases.tui/src/main.rs (2)
158-171:⚠️ Potential issue | 🟠 MajorGuard terminal setup with RAII, not just the panic hook.
Any
?betweenenable_raw_mode()and the explicit cleanup path can return before the hook helps, leaving the shell in raw mode / alternate screen.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tui/src/main.rs` around lines 158 - 171, The terminal setup must be protected by an RAII guard instead of relying only on the panic hook: create a small guard type (e.g. TerminalGuard) whose constructor calls enable_raw_mode() and EnterAlternateScreen and whose Drop impl calls LeaveAlternateScreen and disable_raw_mode(), then instantiate this guard before creating the CrosstermBackend/Terminal (symbols: enable_raw_mode, disable_raw_mode, EnterAlternateScreen, LeaveAlternateScreen, CrosstermBackend::new, Terminal::new) so any early-return or error will run Drop and restore the terminal; keep or call std::panic::set_hook as before, but remove cleanup logic from the hook since the guard handles restoration.
197-199:⚠️ Potential issue | 🟠 MajorParse the port from a URL, not by splitting on
:.
rsplit(':')misparses values likehttp://localhost:31000/healthas31000/health, so auto-start falls back to the default port and may launch SMG on the wrong socket.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tui/src/main.rs` around lines 197 - 199, The current extract_port function incorrectly slices on ':' and misparses paths like "http://localhost:31000/health"; change extract_port to actually parse the URL (use the url::Url parser) and return the port via Url::port() or Url::port_or_known_default() to get the numeric port correctly; update Cargo.toml to include the url crate if missing and replace the rsplit-based logic in extract_port with a proper Url::parse(url) flow that returns the parsed port as Option<u16>.tui/src/ui/pulse.rs (2)
376-389:⚠️ Potential issue | 🟠 MajorUse request-rate history in the compact throughput panel.
This panel reads
throughput_historybut labels itreq/s. It also showsNo datawhenever tok/s history is empty, even if request-rate history exists.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tui/src/ui/pulse.rs` around lines 376 - 389, The compact throughput panel is wrongly using state.throughput_history (and showing "No data" when tok/s history is empty) but should use the request-rate history; update the panel logic in pulse.rs to check state.request_rate_history for emptiness, use state.request_rate_history.back().copied().unwrap_or(0.0) for latest, and replace any uses of state.throughput_history in this panel (rendering the Paragraph and any sparkline) with state.request_rate_history so the label "req/s" matches the actual data shown.
21-38:⚠️ Potential issue | 🟠 MajorKeep Request Stats in the narrow layout.
The
<80branch returns after rendering worker health / node status / throughput, so latency, connections, and in-flight metrics disappear entirely on small terminals.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tui/src/ui/pulse.rs` around lines 21 - 38, The narrow-layout branch for width < 80 omits the request-stats panel (latency/connections/in-flight); update the branch that builds constraints and renders rows so it includes a Constraint and a call to the request-stats renderer (e.g., add Constraint::Fill(1) to the constraints Vec and call render_request_stats(f, &state, rows[i]) in sequence before returning), making sure to account for has_node_panel when computing the row index similarly to render_worker_health, render_node_status, and render_throughput_compact.tui/src/ui/detail.rs (3)
32-34: 🧹 Nitpick | 🔵 TrivialUse
INVARIANT:for this safe-code assumption.This comment documents a lock-poisoning assumption, not an unsafe block.
Based on learnings: "In Rust code across the repository, use the marker INVARIANT: to document assumptions in safe code. Reserve SAFETY: for explaining why unsafe blocks are sound."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tui/src/ui/detail.rs` around lines 32 - 34, Replace the "Safety:" note above the unwrap with the repository's invariant marker: change the comment before the call to app.state.read().unwrap() to start with "INVARIANT:" (e.g., "INVARIANT: RwLock is not poisoned — no panics while holding the lock") so the assumption is documented as a safe-code invariant rather than a SAFETY explanation; leave the #[expect(clippy::unwrap_used)] and the unwrap call unchanged.
225-230:⚠️ Potential issue | 🔴 CriticalMake
truncate_strUTF-8 safe.
&s[..max.saturating_sub(1)]panics whenmaxlands inside a multibyte character, which can crash the detail panel on non-ASCII worker IDs.Proposed fix
fn truncate_str(s: &str, max: usize) -> String { - if s.len() <= max { + if max == 0 { + String::new() + } else if s.chars().count() <= max { s.to_string() } else { - format!("{}…", &s[..max.saturating_sub(1)]) + let prefix: String = s.chars().take(max.saturating_sub(1)).collect(); + format!("{prefix}…") } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tui/src/ui/detail.rs` around lines 225 - 230, truncate_str currently slices bytes which can split UTF-8 multibyte characters; update truncate_str to operate on char boundaries: check if s.chars().count() <= max then return s.to_string(), otherwise build the prefix with s.chars().take(max.saturating_sub(1)).collect::<String>() and append the ellipsis (e.g., prefix + "…"); handle the max == 0 case by returning "…" (or an empty string plus ellipsis) so no byte-slice is ever used and multibyte characters are preserved.
130-143:⚠️ Potential issue | 🟠 MajorDon't proxy circuit-breaker state through
worker.is_healthy.This renders renamed health status, not breaker status. A healthy worker with an open breaker will show as
closed, and an unhealthy worker will show asopen. Use the real breaker signal here, or renderunknownuntil that data is available.Based on learnings: "
healthy_only = trueintentionally separates circuit-breaker-open workers from genuinely unhealthy workers; the 503 path is specifically for healthy workers whose circuit breaker is open."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tui/src/ui/detail.rs` around lines 130 - 143, The UI is incorrectly using worker.is_healthy to represent circuit-breaker state (cb_label/cb_color and the "Circuit: " span), causing healthy-but-open breakers to display as "closed"; update this block to read the actual breaker signal (e.g., a field like worker.circuit_open, worker.breaker_state, or similar) when available and map that to labels/colors, and if no breaker state is present render "unknown" instead; also keep the existing health-only semantics (healthy_only) intact so the 503 path still applies for healthy workers with an open breaker.tui/src/ui/action_menu.rs (1)
145-167:⚠️ Potential issue | 🟠 MajorRemove the per-frame
Box::leakallocations in the model picker.This branch runs on every render while
SelectModelis open, so the leaked numbering strings grow the heap until the process exits. Keep the indices owned and render fromString/Cowinstead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tui/src/ui/action_menu.rs` around lines 145 - 167, The code leaks heap strings via Box::leak in AddMenuState::SelectModel; replace those leaks by keeping owned Strings (or Cow) for the index labels so they live for the duration of the render and can be borrowed when building refs. Concretely, change items to Vec<(String, String, String)> (build index labels with format!("{}", i+1) and the custom label with format!("{}", presets.len()+1)), push those Strings into items, then create refs by borrowing with .as_str() (e.g. .map(|(n,l,d)| (n.as_str(), l.as_str(), d.as_str())) ) before calling render_menu; this removes Box::leak while keeping the same render_menu call pattern.tui/src/app.rs (5)
633-635:⚠️ Potential issue | 🟠 MajorPressing
qquits even while typing in the chat input.In
handle_chat_key, pressingqwhen!self.chat_streamingalways setsshould_quit = true. Unlike other view-switching keys (lines 647-659) which checkself.chat_input.is_empty(), this prevents typing the letter 'q' in chat messages.Proposed fix
- KeyCode::Char('q') if !self.chat_streaming => { + KeyCode::Char('q') if !self.chat_streaming && self.chat_input.is_empty() => { self.should_quit = true; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tui/src/app.rs` around lines 633 - 635, In handle_chat_key, the KeyCode::Char('q') branch sets self.should_quit = true unconditionally when !self.chat_streaming, preventing typing 'q' into the chat; change the condition to mirror the other view-switching keys by requiring both !self.chat_streaming and self.chat_input.is_empty() before setting self.should_quit, so that 'q' only quits when the chat input is empty and not streaming.
1281-1288:⚠️ Potential issue | 🟠 MajorRoll back spawned worker when gateway registration fails.
When
add_worker()fails at line 1273, the child process is already inworker_children(line 1253) and GPUs are claimed (line 1262). The process continues running and claims are never released.Proposed fix: Clean up on registration failure
Err(e) => { self.add_log( LogLevel::Error, &format!("Failed to register worker {url}: {e}"), ); + // Roll back: release GPU claim and kill the spawned process + self.claimed_gpus.remove(&url); + if let Some(pos) = self.worker_children.iter().position(|(d, _)| d == &desc) { + let (_, mut child) = self.worker_children.remove(pos); + let _ = child.start_kill(); + } self.set_status(format!("Started {desc} but registration failed: {e}")); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tui/src/app.rs` around lines 1281 - 1288, On registration failure in the Err(e) branch after calling add_worker(), ensure you roll back the spawned worker: remove its entry from worker_children, terminate/kill the child process (or send shutdown) and release any claimed GPUs (the same logic used when stopping workers), then log and set status as before; implement this cleanup in the Err(e) block that currently calls add_log and set_status so the child process doesn't keep running and GPUs remain freed.
758-774:⚠️ Potential issue | 🟠 MajorExternal worker model filtering excludes valid provider models.
The
gpt-5.4prefix check drops models from Anthropic, xAI, Gemini, and most OpenAI models (e.g.,gpt-4o,claude-3,gemini-1.5-pro), making them unselectable in the chat tab.Consider using the models actually reported by each worker, and only apply a provider-specific default for true wildcard workers (empty model list).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tui/src/app.rs` around lines 758 - 774, The current loop over workers (workers.workers) treats any external worker as requiring gpt-5.4-prefixed models by filtering with m.id.starts_with("gpt-5.4"), which drops valid provider model ids; change the logic so that if a worker has an empty models list (w.models.is_empty()) you add the provider-default placeholder "gpt-5.4-nano" once, but otherwise accept and push every reported model id (m.id) for that worker without applying the starts_with filter; specifically remove the branch that does if is_external && !m.id.starts_with("gpt-5.4") { continue } and keep the existing duplicate-check on models.contains before pushing m.id, and ensure the placeholder insertion still guards against duplicates.
534-556:⚠️ Potential issue | 🟠 Major
toggle-healthcommand only disables health checks, never re-enables.Both the
:toggle-healthcommand here and theToggleHealthCheckmenu action at lines 904-930 always senddisable_health_check: Some(true). This is a one-way action that cannot re-enable health checks.Either derive the next state from the worker's current
disable_health_checkvalue, or rename to:disable-health/ "Disable health check" to clarify the one-way behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tui/src/app.rs` around lines 534 - 556, The toggle-health handler (and the ToggleHealthCheck menu action) always sends disable_health_check: Some(true), making it one-way; change the logic to read the worker's current disable_health_check state (e.g., fetch the worker via self.client.get_worker(&id).await or from the in-memory worker record) and send disable_health_check: Some(!current_value) when building openai_protocol::worker::WorkerUpdateRequest so the update toggles between enabled/disabled; update both the "toggle-health" match arm and the ToggleHealthCheck action to use the same toggle-by-current-state approach and preserve other fields as before.
1156-1168:⚠️ Potential issue | 🟡 MinorTOCTOU race on ephemeral port allocation.
The
TcpListenerbinds to get a free port, then immediately drops. Between dropping and the worker process binding, another process could claim that port.While unlikely in practice, consider either:
- Passing port 0 directly to the worker runtime if it supports reporting the actual bound port
- Implementing retry logic if the worker fails to bind
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tui/src/app.rs` around lines 1156 - 1168, The current ephemeral-port allocation uses std::net::TcpListener to pick a free port then immediately drops it (the match that assigns port), creating a TOCTOU race; instead either (A) stop pre-binding and pass port 0 directly to the worker runtime so the worker itself binds and reports the actual bound port, or (B) add robust retry logic around worker bind attempts: if you must keep the existing TcpListener pattern (the code that assigns port and calls self.set_status on error), modify the worker spawn/bind path to accept port 0 or detect binding failure and retry N times with a small backoff before reporting failure via self.set_status; reference the current port variable and the TcpListener bind block when implementing this change so the worker binding and reporting are atomic and race-free.tui/src/state.rs (2)
450-466:⚠️ Potential issue | 🟡 MinorPotential negative latency on Prometheus counter reset.
Line 453 uses plain subtraction (
cur_sum - prev_sum) which can yield a negative value if the Prometheus server restarts and counters reset to zero. This pushes negative latency values toavg_latency_history.Proposed fix: Clamp delta_sum to non-negative
let (cur_sum, cur_count) = parse_duration_stats(&metrics_text); if let (Some(prev_sum), Some(prev_count)) = (s.prev_duration_sum, s.prev_duration_count) { - let delta_sum = cur_sum - prev_sum; + let delta_sum = if cur_sum >= prev_sum { cur_sum - prev_sum } else { 0.0 }; let delta_count = cur_count.saturating_sub(prev_count);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tui/src/state.rs` around lines 450 - 466, The subtraction cur_sum - prev_sum in the avg latency computation can be negative after a Prometheus counter reset; in parse_duration_stats usage where you compute delta_sum and delta_count (using s.prev_duration_sum and s.prev_duration_count) clamp delta_sum to non-negative before computing avg_latency (e.g., replace raw subtraction with a non-negative max) so you never push negative values into s.avg_latency_history (respect existing delta_count handling and SPARKLINE_CAP).
395-427:⚠️ Potential issue | 🔴 Critical
throughput_historyreceives duplicate entries when both conditions are met.When worker metrics are available (
has_worker_metricsis true) AND Prometheus metrics fetch succeeds,throughput_historygets pushed twice per poll cycle:
- Line 398: pushes
total_throughput(from loads API)- Line 427: pushes
rps(from Prometheus counter)This corrupts the sparkline data and causes the rolling history to advance twice as fast as intended.
Proposed fix: Use req/s as sole source when available
if has_worker_metrics { // ... per-worker metrics processing ... - if s.throughput_history.len() >= SPARKLINE_CAP { - s.throughput_history.pop_front(); - } - s.throughput_history.push_back(total_throughput); - let avg_cache = if worker_count > 0 {This keeps the Prometheus-based
rpsas the primary throughput source (lines 424-427), which works for all worker types including external providers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tui/src/state.rs` around lines 395 - 427, The throughput_history is being pushed twice when both worker metrics and Prometheus metrics are present; modify the update logic so that when Prometheus metrics fetch (metrics) is Ok and parse_request_count yields an rps, you treat rps as the sole throughput source and do not push total_throughput into s.throughput_history. Concretely, in the block that currently pushes total_throughput (using total_throughput and has_worker_metrics), guard that push so it only runs when metrics is Err/unavailable (or when prev_request_count yields None), and keep the existing push of rps (from parse_request_count / current_count / delta) as the primary update for s.throughput_history and s.requests_per_sec_history.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tui/src/chat.rs`:
- Around line 198-237: The SSE parser in process_sse_stream currently only
extracts choices[0].delta.content and treats EOF as success; update it to detect
SSE event types and surface mid-stream errors: while parsing lines in
process_sse_stream, check for "event: " lines (e.g., event: error) and when an
"error" event is seen, read the following "data: " payload, parse it for an
error message (or send the raw data) and send it over tx as an error (e.g.,
prefix with [ERROR]) and then return without emitting [DONE]; also when a "data:
" payload parses to a JSON object containing an "error" field, surface that as
an error immediately instead of ignoring it. Use the existing buffer/line
handling and tx sending flow in process_sse_stream and avoid emitting [DONE]
after an error.
In `@tui/src/main.rs`:
- Around line 108-116: The readiness loop currently times out after 30s; change
the timeout to 120s so auto-start waits the full intended period. Replace
Duration::from_secs(30) with Duration::from_secs(120) in the deadline
calculation (the code around tokio::time::Instant::now() +
tokio::time::Duration::from_secs(...)), and update the tracing::warn message if
desired to reflect 120s so Gateway readiness polling for auto-start matches the
README/PR intent.
In `@tui/src/types.rs`:
- Around line 162-187: The Vllm branch (Self::Vllm) builds a gRPC entrypoint
("vllm.entrypoints.grpc_server") that requires vLLM >= 0.14.0 when grpc is true;
add a runtime version check before constructing the command (or explicitly
document the requirement) — e.g., when handling Self::Vllm and grpc == true,
query the installed vllm version from the Python environment and error/abort
with a clear message if it's < 0.14.0, otherwise proceed to set entrypoint and
args (variables: entrypoint, grpc, model_id, tp, port) so the launcher fails
fast with a clear diagnostic instead of attempting to start an unsupported
entrypoint.
---
Duplicate comments:
In `@tui/src/app.rs`:
- Around line 633-635: In handle_chat_key, the KeyCode::Char('q') branch sets
self.should_quit = true unconditionally when !self.chat_streaming, preventing
typing 'q' into the chat; change the condition to mirror the other
view-switching keys by requiring both !self.chat_streaming and
self.chat_input.is_empty() before setting self.should_quit, so that 'q' only
quits when the chat input is empty and not streaming.
- Around line 1281-1288: On registration failure in the Err(e) branch after
calling add_worker(), ensure you roll back the spawned worker: remove its entry
from worker_children, terminate/kill the child process (or send shutdown) and
release any claimed GPUs (the same logic used when stopping workers), then log
and set status as before; implement this cleanup in the Err(e) block that
currently calls add_log and set_status so the child process doesn't keep running
and GPUs remain freed.
- Around line 758-774: The current loop over workers (workers.workers) treats
any external worker as requiring gpt-5.4-prefixed models by filtering with
m.id.starts_with("gpt-5.4"), which drops valid provider model ids; change the
logic so that if a worker has an empty models list (w.models.is_empty()) you add
the provider-default placeholder "gpt-5.4-nano" once, but otherwise accept and
push every reported model id (m.id) for that worker without applying the
starts_with filter; specifically remove the branch that does if is_external &&
!m.id.starts_with("gpt-5.4") { continue } and keep the existing duplicate-check
on models.contains before pushing m.id, and ensure the placeholder insertion
still guards against duplicates.
- Around line 534-556: The toggle-health handler (and the ToggleHealthCheck menu
action) always sends disable_health_check: Some(true), making it one-way; change
the logic to read the worker's current disable_health_check state (e.g., fetch
the worker via self.client.get_worker(&id).await or from the in-memory worker
record) and send disable_health_check: Some(!current_value) when building
openai_protocol::worker::WorkerUpdateRequest so the update toggles between
enabled/disabled; update both the "toggle-health" match arm and the
ToggleHealthCheck action to use the same toggle-by-current-state approach and
preserve other fields as before.
- Around line 1156-1168: The current ephemeral-port allocation uses
std::net::TcpListener to pick a free port then immediately drops it (the match
that assigns port), creating a TOCTOU race; instead either (A) stop pre-binding
and pass port 0 directly to the worker runtime so the worker itself binds and
reports the actual bound port, or (B) add robust retry logic around worker bind
attempts: if you must keep the existing TcpListener pattern (the code that
assigns port and calls self.set_status on error), modify the worker spawn/bind
path to accept port 0 or detect binding failure and retry N times with a small
backoff before reporting failure via self.set_status; reference the current port
variable and the TcpListener bind block when implementing this change so the
worker binding and reporting are atomic and race-free.
In `@tui/src/event.rs`:
- Around line 31-32: Change the comment prefix from "Safety:" to "INVARIANT:"
for the safe-code assumption in the event reader description so it follows
repository convention; update the comment that currently reads "// Safety:
fire-and-forget event reader loop that runs for the app's lifetime" to use
"INVARIANT:" instead (this is right above the
#[expect(clippy::disallowed_methods)] attribute in event.rs and refers to the
event reader loop assumption).
- Line 1: The event forwarder is emitting all crossterm Event::Key variants
causing duplicate actions on Repeat/Release; update the code that matches
Event::Key (in tui/src/event.rs) to only enqueue/forward when the contained
KeyEvent has kind == KeyEventKind::Press (i.e., filter Event::Key(k) by k.kind
== KeyEventKind::Press) so the rest of the app receives only key presses.
In `@tui/src/main.rs`:
- Around line 158-171: The terminal setup must be protected by an RAII guard
instead of relying only on the panic hook: create a small guard type (e.g.
TerminalGuard) whose constructor calls enable_raw_mode() and
EnterAlternateScreen and whose Drop impl calls LeaveAlternateScreen and
disable_raw_mode(), then instantiate this guard before creating the
CrosstermBackend/Terminal (symbols: enable_raw_mode, disable_raw_mode,
EnterAlternateScreen, LeaveAlternateScreen, CrosstermBackend::new,
Terminal::new) so any early-return or error will run Drop and restore the
terminal; keep or call std::panic::set_hook as before, but remove cleanup logic
from the hook since the guard handles restoration.
- Around line 197-199: The current extract_port function incorrectly slices on
':' and misparses paths like "http://localhost:31000/health"; change
extract_port to actually parse the URL (use the url::Url parser) and return the
port via Url::port() or Url::port_or_known_default() to get the numeric port
correctly; update Cargo.toml to include the url crate if missing and replace the
rsplit-based logic in extract_port with a proper Url::parse(url) flow that
returns the parsed port as Option<u16>.
In `@tui/src/state.rs`:
- Around line 450-466: The subtraction cur_sum - prev_sum in the avg latency
computation can be negative after a Prometheus counter reset; in
parse_duration_stats usage where you compute delta_sum and delta_count (using
s.prev_duration_sum and s.prev_duration_count) clamp delta_sum to non-negative
before computing avg_latency (e.g., replace raw subtraction with a non-negative
max) so you never push negative values into s.avg_latency_history (respect
existing delta_count handling and SPARKLINE_CAP).
- Around line 395-427: The throughput_history is being pushed twice when both
worker metrics and Prometheus metrics are present; modify the update logic so
that when Prometheus metrics fetch (metrics) is Ok and parse_request_count
yields an rps, you treat rps as the sole throughput source and do not push
total_throughput into s.throughput_history. Concretely, in the block that
currently pushes total_throughput (using total_throughput and
has_worker_metrics), guard that push so it only runs when metrics is
Err/unavailable (or when prev_request_count yields None), and keep the existing
push of rps (from parse_request_count / current_count / delta) as the primary
update for s.throughput_history and s.requests_per_sec_history.
In `@tui/src/ui/action_menu.rs`:
- Around line 145-167: The code leaks heap strings via Box::leak in
AddMenuState::SelectModel; replace those leaks by keeping owned Strings (or Cow)
for the index labels so they live for the duration of the render and can be
borrowed when building refs. Concretely, change items to Vec<(String, String,
String)> (build index labels with format!("{}", i+1) and the custom label with
format!("{}", presets.len()+1)), push those Strings into items, then create refs
by borrowing with .as_str() (e.g. .map(|(n,l,d)| (n.as_str(), l.as_str(),
d.as_str())) ) before calling render_menu; this removes Box::leak while keeping
the same render_menu call pattern.
In `@tui/src/ui/chat.rs`:
- Around line 22-37: The title shows an incorrect path for ChatEndpoint::Chat;
update the title formatting in chat UI to display the actual POST path used by
the client ("/v1/chat/completions") instead of "/v1/chat" when app.chat_endpoint
is Chat; locate the code that builds the title (uses app.chat_endpoint,
ChatEndpoint::Chat and app.chat_endpoint.label()) and either change the label()
call for Chat to return "chat/completions" or special-case Chat in this
formatting to insert "/v1/chat/completions" so the displayed path matches the
request path used by tui/src/chat.rs.
- Around line 152-164: The scroll math is using byte length via
span.content.len(), which miscalculates display width for emoji/CJK; change the
width calculation in the total_lines mapping to use a display-width function
(e.g. UnicodeWidthStr::width) on each span's content instead of len(), keep the
max(1) and ceil division logic, and add the necessary import for the
unicode-width helper so total_lines reflects visual columns rather than byte
count.
In `@tui/src/ui/detail.rs`:
- Around line 32-34: Replace the "Safety:" note above the unwrap with the
repository's invariant marker: change the comment before the call to
app.state.read().unwrap() to start with "INVARIANT:" (e.g., "INVARIANT: RwLock
is not poisoned — no panics while holding the lock") so the assumption is
documented as a safe-code invariant rather than a SAFETY explanation; leave the
#[expect(clippy::unwrap_used)] and the unwrap call unchanged.
- Around line 225-230: truncate_str currently slices bytes which can split UTF-8
multibyte characters; update truncate_str to operate on char boundaries: check
if s.chars().count() <= max then return s.to_string(), otherwise build the
prefix with s.chars().take(max.saturating_sub(1)).collect::<String>() and append
the ellipsis (e.g., prefix + "…"); handle the max == 0 case by returning "…" (or
an empty string plus ellipsis) so no byte-slice is ever used and multibyte
characters are preserved.
- Around line 130-143: The UI is incorrectly using worker.is_healthy to
represent circuit-breaker state (cb_label/cb_color and the "Circuit: " span),
causing healthy-but-open breakers to display as "closed"; update this block to
read the actual breaker signal (e.g., a field like worker.circuit_open,
worker.breaker_state, or similar) when available and map that to labels/colors,
and if no breaker state is present render "unknown" instead; also keep the
existing health-only semantics (healthy_only) intact so the 503 path still
applies for healthy workers with an open breaker.
In `@tui/src/ui/footer.rs`:
- Line 25: The footer currently advertises only "1-5" in the normal-mode hint
sets, but the UI exposes seven views; update the two hint label calls so the
footer advertises all seven views. Locate the hint("1-5", "view") invocations in
tui/src/ui/footer.rs (the two occurrences around the normal-mode hint blocks)
and change the displayed range to "1-7" (e.g., hint("1-7", "view")) so both
normal-mode hint sets advertise all seven views including Traffic and Mesh.
In `@tui/src/ui/logs.rs`:
- Around line 163-190: render_file_log currently reads and parses the entire
file on every frame (via std::fs::read_to_string and content.lines().collect),
which blocks the UI; move all file I/O and tailing into the background poller
and have render_file_log only render a cached tail. Specifically: add a cached
tail field (e.g., App.cached_log_tail: Vec<Line> or String) updated by the
poller, change the poller to read the file, compute the last N lines
(max_lines=500), and store the result into that cache using a thread-safe
mechanism (Arc<Mutex<...>> or a channel update on App), and modify
render_file_log to skip read_to_string and instead render from
App.cached_log_tail (still using the same Block/Paragraph rendering and the
existing title/label logic).
In `@tui/src/ui/models.rs`:
- Line 18: Header text "OWNER" in the table (created via Row::new in the header
variable) does not match the rendered cell value which uses m.display_name;
change one to match the other by either renaming the header to "NAME" (or "OWNER
/ NAME") where header is built in Row::new, or update the cell renderer to
display the actual owner field (e.g., replace m.display_name with m.owner or
m.owner_id) in the code that builds the row cells around where m is used (lines
rendering the row, e.g., the code around m.display_name). Ensure the header
label and the value source are consistent across the table rendering.
In `@tui/src/ui/pulse.rs`:
- Around line 376-389: The compact throughput panel is wrongly using
state.throughput_history (and showing "No data" when tok/s history is empty) but
should use the request-rate history; update the panel logic in pulse.rs to check
state.request_rate_history for emptiness, use
state.request_rate_history.back().copied().unwrap_or(0.0) for latest, and
replace any uses of state.throughput_history in this panel (rendering the
Paragraph and any sparkline) with state.request_rate_history so the label
"req/s" matches the actual data shown.
- Around line 21-38: The narrow-layout branch for width < 80 omits the
request-stats panel (latency/connections/in-flight); update the branch that
builds constraints and renders rows so it includes a Constraint and a call to
the request-stats renderer (e.g., add Constraint::Fill(1) to the constraints Vec
and call render_request_stats(f, &state, rows[i]) in sequence before returning),
making sure to account for has_node_panel when computing the row index similarly
to render_worker_health, render_node_status, and render_throughput_compact.
In `@tui/src/ui/stats_bar.rs`:
- Around line 79-85: The health_text branch logic treats a connected gateway
with total == 0 as "all healthy"; change health_text (used where
state.connected, unhealthy, total are referenced) so that when state.connected
is true and total == 0 it returns the neutral/no-workers variant (e.g., "--" or
"no workers" with theme::TEXT_MUTED) before checking unhealthy == 0; reorder the
conditions accordingly so the total==0 case is handled first while preserving
the existing GREEN for actual healthy workers and RED for unhealthy cases.
In `@tui/src/ui/tabs.rs`:
- Around line 14-18: The tab numbering currently uses enumerate() and i + 1 when
building tabs from View::all(), which duplicates View's own numbering logic;
update the closure that constructs the tab label (the block producing num =
format!("{}", i + 1)) to use view.index() as the source of the tab number
instead (call view.index() and format that value), removing reliance on
enumerate() for numbering so the rendered shortcut stays consistent with
View::index() across the codebase.
In `@tui/src/ui/workers.rs`:
- Around line 299-304: The truncate(s: &str, max: usize) function currently
slices by bytes which can panic on UTF-8 boundaries; change it to treat max as a
character limit by operating on chars rather than byte indices (e.g., iterate
s.chars() and collect up to max characters, appending the ellipsis when the
original string has more than max chars). Update truncate to check character
count, return s.to_string() when length <= max, otherwise build a new String
from the first max-1 characters (or first max and then replace last with
ellipsis per existing behavior) using char iteration so all slicing is UTF-8
safe; reference function name truncate in this file.
- Around line 269-283: The detail pane currently uses the raw app.selected_index
which can be out-of-sync with the clamped selection used for the table; after
you call table_state.select(...) read the clamped index from
table_state.selected() and use that to look up the worker for
detail::render_detail instead of app.selected_index. Concretely, after building
table_state use something like if let Some(clamped) = table_state.selected() {
if let Some(worker) = filtered.get(clamped) { detail::render_detail(f, app,
worker, detail_area); } } so the detail pane always follows the table's actual
highlighted row.
- Around line 326-346: The current code drops extra load entries by using
details.loads.first() and also fails to use worker_rps for plain "http://"
fallbacks; change the logic in the worker-load formatting block to (1) aggregate
across all entries in details.loads (e.g., total_running =
sum(load.num_running_reqs) and token_usage = max(load.token_usage) or a sensible
aggregate) instead of details.loads.first(), and (2) move/extend the
rps-fallback to cover plain "http://" (or, more generally, if worker_rps
contains an entry use that) so that when /get_loads is missing you return
(format!("{rps:.1} r/s"), "N/A".to_string()) rather than ("0", "0.0%").
Reference symbols: loads.get(worker_url), wl.details, details.loads,
worker_url.starts_with(...), and worker_rps.get(worker_url).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7a504163-0ef8-4bf8-b21d-b565d83a73ed
⛔ Files ignored due to path filters (2)
tui/assets/add-worker.gifis excluded by!**/*.giftui/assets/tui-demo.gifis excluded by!**/*.gif
📒 Files selected for processing (28)
.pre-commit-config.yamlCargo.tomltui/Cargo.tomltui/README.mdtui/src/app.rstui/src/chat.rstui/src/client.rstui/src/event.rstui/src/lib.rstui/src/main.rstui/src/state.rstui/src/types.rstui/src/ui/action_menu.rstui/src/ui/chat.rstui/src/ui/detail.rstui/src/ui/dialog.rstui/src/ui/filter.rstui/src/ui/footer.rstui/src/ui/help.rstui/src/ui/logs.rstui/src/ui/mod.rstui/src/ui/models.rstui/src/ui/pulse.rstui/src/ui/sparkline.rstui/src/ui/stats_bar.rstui/src/ui/tabs.rstui/src/ui/theme.rstui/src/ui/workers.rs
key4ng
force-pushed
the
keyang/tui-redesign
branch
from
March 23, 2026 20:00
262575f to
d4bf248
Compare
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (6)
tui/src/event.rs (1)
31-32: 🧹 Nitpick | 🔵 TrivialUse
INVARIANT:instead ofSafety:for non-unsafe code.The comment on Line 31 uses "Safety:" but this isn't an
unsafeblock. Per repository conventions, useINVARIANT:to document assumptions in safe code.📝 Proposed fix
- // Safety: fire-and-forget event reader loop that runs for the app's lifetime + // INVARIANT: fire-and-forget event reader loop that runs for the app's lifetimeBased on learnings: "In Rust code across the repository, use the marker INVARIANT: to document assumptions in safe code. Reserve SAFETY: for explaining why unsafe blocks are sound."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tui/src/event.rs` around lines 31 - 32, The comment above the event reader loop currently uses "Safety:" but this is safe code; update the doc comment to use "INVARIANT:" instead to follow repository conventions—locate the comment immediately preceding the #[expect(clippy::disallowed_methods)] attribute and the fire-and-forget event reader loop and replace "Safety:" with "INVARIANT:" so the assumption is documented correctly for the safe code path.tui/src/ui/workers.rs (1)
345-356:⚠️ Potential issue | 🟡 MinorPlain
http://workers don't fall back to Prometheus RPS.Lines 346-354 provide
worker_rpsfallback forgrpc://andhttps://workers when/get_loadsdata is unavailable, but plainhttp://workers fall through to line 355 returning("0", "0.0%"). If local sglang/vllm workers usehttp://URLs, they'd show zero load when/get_loadsis temporarily unavailable instead of the Prometheus-derived request rate.🩹 Proposed fix
// External workers if worker_url.starts_with("https://") { let rps = worker_rps.get(worker_url).copied().unwrap_or(0.0); return (format!("{rps:.1} r/s"), "N/A".to_string()); } - ("0".to_string(), "0.0%".to_string()) + // Fallback for http:// and other schemes: use Prometheus RPS if available + if worker_url.starts_with("http://") { + let rps = worker_rps.get(worker_url).copied().unwrap_or(0.0); + return (format!("{rps:.1} r/s"), "N/A".to_string()); + } + ("--".to_string(), "--".to_string()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tui/src/ui/workers.rs` around lines 345 - 356, The code only checks worker_url prefixes "grpc://" and "https://" for Prometheus RPS fallback, causing plain "http://" workers to return ("0","0.0%"); update the logic that builds the per-worker load tuple (the block using worker_url and worker_rps) to also handle "http://" (either add an if branch for worker_url.starts_with("http://") mirroring the grpc/https branches or collapse into a single branch that checks any scheme and uses worker_rps.get(worker_url).copied().unwrap_or(0.0) to format "{rps:.1} r/s" and "N/A"), referencing the existing worker_url and worker_rps variables to locate and change the code.tui/src/main.rs (2)
108-116:⚠️ Potential issue | 🟡 MinorReadiness timeout is 30s but documentation mentions 120s.
The PR objectives/README describe readiness polling for up to 120s, but this code times out after 30s. For slow local model loading, 30s may be insufficient.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tui/src/main.rs` around lines 108 - 116, The readiness timeout is set to 30s but the docs state 120s; update the timeout in the loop where deadline is computed (the tokio::time::Instant::now() + tokio::time::Duration::from_secs(30) assignment that defines deadline) to 120 seconds (or replace the literal with a named constant like READYNESS_TIMEOUT_SECS used by the readiness polling logic) so the loop waits up to 120s before warning and breaking.
208-211:⚠️ Potential issue | 🟠 Major
extract_port()misparses URLs that include a path.For inputs like
http://localhost:31000/health,rsplit(':')yields31000/health, so the parse fails and auto-start falls back to the default port. Consider proper URL parsing.Proposed fix
/// Extract port from a URL like "http://localhost:30000". fn extract_port(url: &str) -> Option<u16> { - url.rsplit(':').next()?.trim_end_matches('/').parse().ok() + // Strip scheme, then extract host:port portion before any path + let without_scheme = url + .trim_start_matches("http://") + .trim_start_matches("https://"); + let host_port = without_scheme.split('/').next()?; + host_port.rsplit(':').next()?.parse().ok() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tui/src/main.rs` around lines 208 - 211, The current extract_port() uses rsplit(':') and fails on URLs with paths (e.g. "http://host:31000/health"); replace that ad-hoc parsing with a proper URL parse: call url::Url::parse(url) (or prepend "http://" if scheme may be missing), then return the port via url.port().map(|p| p) or url.port_or_known_default() wrapped as Option<u16>, and handle parse errors by returning None; update the extract_port function to use url::Url parsing and error-safe extraction instead of rsplit.tui/src/ui/detail.rs (1)
32-34: 🧹 Nitpick | 🔵 TrivialUse
INVARIANT:instead ofSafety:for non-unsafe code.Line 32 uses "Safety:" but this is a safe
RwLock::read().unwrap()call, not anunsafeblock. Per repository conventions, useINVARIANT:for documenting assumptions in safe code.Proposed fix
- // Safety: RwLock is not poisoned — no panics while holding the lock + // INVARIANT: RwLock is not poisoned — no panics while holding the lockBased on learnings: "In Rust code across the repository, use the marker INVARIANT: to document assumptions in safe code. Reserve SAFETY: for explaining why unsafe blocks are sound."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tui/src/ui/detail.rs` around lines 32 - 34, The comment above the RwLock read should use the repository convention INVARIANT: instead of Safety: because this is documenting an assumption in safe code; update the doc comment that precedes #[expect(clippy::unwrap_used)] and the line let state = app.state.read().unwrap() to start with "INVARIANT:" (e.g., "INVARIANT: RwLock is not poisoned — no panics while holding the lock") so it matches the project's guidance for non-unsafe code.tui/src/app.rs (1)
451-466:⚠️ Potential issue | 🟠 Major
:delete <id>uses wrong worker URL for cleanup and doesn't kill backend.The command deletes the worker by the provided ID argument, but then calls
selected_worker_url()which returns the currently selected worker's URL—not the worker being deleted. This causes:
- GPU claims for the wrong worker get released
- The deleted worker's backend process is never killed
- The deleted worker's GPU claims are never released
The interactive delete (lines 829-882) correctly uses the confirmed worker's URL and kills the backend. This command should do the same.
Proposed fix: Look up worker URL by ID before deletion
Some("delete") => { if let Some(id) = parts.get(1) { let id = id.trim().to_string(); + // Look up worker URL before deletion + let worker_url = { + #[expect(clippy::unwrap_used)] + let state = self.state.read().unwrap(); + state.workers.as_ref() + .and_then(|wl| wl.workers.iter().find(|w| w.id == id)) + .map(|w| w.url.clone()) + }; match self.client.delete_worker(&id).await { Ok(_) => { - // Also clean up spawned process and GPU claims (like interactive delete) - let worker_url = self.selected_worker_url().unwrap_or_default(); - self.claimed_gpus.remove(&worker_url); + if let Some(url) = worker_url { + // Release claimed GPUs + if self.claimed_gpus.remove(&url).is_some() { + self.add_log(LogLevel::Info, &format!("Released GPU claim for {url}")); + } + // Kill backend process if spawned by TUI + if let Some(port) = url.rsplit(':').next() { + let port = port.trim_matches('/').to_string(); + let match_str = format!("port {port}"); + if let Some(pos) = self.worker_children.iter().position(|(d, _)| d.contains(&match_str)) { + let (desc, mut child) = self.worker_children.remove(pos); + let _ = child.kill().await; + self.add_log(LogLevel::Info, &format!("Killed backend: {desc}")); + } + } + } self.set_status(format!("Worker {id} deleted")); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tui/src/app.rs` around lines 451 - 466, When handling the "delete" command, don't call selected_worker_url(); instead look up the worker's URL by the provided id (the same lookup used in the interactive delete flow), store it in a local worker_url before calling self.client.delete_worker(&id).await, then after a successful delete call the same backend-kill routine used by the interactive delete to terminate the worker process and remove GPU claims from self.claimed_gpus for that worker_url, and finally call self.set_status with the id; mirror the interactive delete's order and method calls so the deleted worker's process is killed and its GPU claims are released rather than touching the currently-selected worker.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tui/src/app.rs`:
- Around line 540-565: The toggle currently uses worker.is_healthy to set
disable_health_check, which is incorrect; change the logic in the
"toggle-health" branch to read the worker's current health.disable_health_check
(e.g., via selected_worker() -> worker.health.disable_health_check), compute
new_disable = !current_disable (treat missing value as false), build the
WorkerUpdateRequest with health.disable_health_check: Some(new_disable) and
update action text to reflect actual change (enabled/disabled based on
new_disable), then call self.client.update_worker(&worker.id, &update) and
set_status as before.
- Around line 1360-1371: clamp_selection currently clamps selected_index against
the unfiltered workers.len(); change it to compute the visible/filtered count
using the same filtering logic the UI uses (the predicate used by
selected_worker or the function that produces the displayed list from
state.workers), then clamp selected_index to visible_count.saturating_sub(1) (or
set to 0 when visible_count==0) so the index is always valid for the displayed
list; update clamp_selection to read state, produce the filtered worker
slice/iterator, get its length, and use that length for the min() bound on
selected_index.
In `@tui/src/state.rs`:
- Around line 306-308: Replace the "Safety:" comment marker with "INVARIANT:"
for the safe RwLock usage to match repository convention; update the comment
above the state.write().unwrap() call (the RwLock usage where the code currently
reads "// Safety: RwLock is not poisoned in practice — no panics while holding
the lock") to use "INVARIANT:" and keep the same explanatory text about
non-poisoning and no panics while holding the lock.
In `@tui/src/ui/action_menu.rs`:
- Around line 249-255: The mask uses input.len() (byte length) causing incorrect
mask width for multi-byte characters; update the branch that constructs the
masked Span (the else-if using `masked` and
`Span::styled("*".repeat(input.len()), ...)`) to compute the character count
with `input.chars().count()` and repeat '*' that many times so the displayed
mask matches the visual character count (keep the other branches and
Style/theme::TEXT usage unchanged).
In `@tui/src/ui/logs.rs`:
- Around line 163-198: The render_file_log function currently seeks to len -
TAIL_BYTES which can land mid-UTF-8 sequence and produce a corrupted first line;
after the seek/read (the branch where len > TAIL_BYTES and file.read_to_string
populates buf) detect that we did a mid-file seek and drop the partial first
line by finding the first newline in buf and replacing buf with the substring
after that newline (or clearing buf if none found) before rendering; update the
code around TAIL_BYTES, the file.seek call, and the subsequent use of
buf/read_to_string to perform this safe-trim-of-first-line so the displayed tail
never begins with a partial UTF-8 character.
In `@tui/src/ui/models.rs`:
- Around line 12-14: Replace the misleading comment marker "Safety:" with the
repository convention "INVARIANT:" for the non-unsafe assumption above the
RwLock read; specifically update the comment before the expect attribute that
documents why app.state.read().unwrap() is safe to read (e.g., change "//
Safety: RwLock is not poisoned — no panics while holding the lock" to use
"INVARIANT:" and keep the rest of the text and the
#[expect(clippy::unwrap_used)] and the call to app.state.read().unwrap()
unchanged).
In `@tui/src/ui/pulse.rs`:
- Around line 242-254: shorten_gpu_name uses byte-slicing name[..12] which can
panic on UTF-8 boundaries; change it to perform char-based truncation similar to
truncate_str by taking the first 12 chars (e.g.,
name.chars().take(12).collect()) after applying the trim_start_matches calls,
and return that String instead of slicing bytes — update the shorten_gpu_name
function to use character iteration to safely truncate GPU names.
- Around line 13-15: Change the comment before the RwLock read to use the
repository convention for safe-code assumptions: replace the "Safety:" marker
with "INVARIANT:" for the `app.state.read().unwrap()` usage (the comment that
documents why unwrapping is safe for RwLock in `pulse.rs`), keeping the existing
#[expect(clippy::unwrap_used)] attribute intact.
In `@tui/src/ui/workers.rs`:
- Around line 23-25: Replace the comment annotating the assumption about the
RwLock with the repository convention: change the leading "Safety:" marker to
"INVARIANT:" for the non-unsafe code that reads the state; locate the read call
(app.state.read().unwrap()) and update the preceding comment from "Safety:
RwLock is not poisoned — no panics while holding the lock" to begin with
"INVARIANT:" while keeping the rest of the explanatory text and preserving the
#[expect(clippy::unwrap_used)] attribute.
---
Duplicate comments:
In `@tui/src/app.rs`:
- Around line 451-466: When handling the "delete" command, don't call
selected_worker_url(); instead look up the worker's URL by the provided id (the
same lookup used in the interactive delete flow), store it in a local worker_url
before calling self.client.delete_worker(&id).await, then after a successful
delete call the same backend-kill routine used by the interactive delete to
terminate the worker process and remove GPU claims from self.claimed_gpus for
that worker_url, and finally call self.set_status with the id; mirror the
interactive delete's order and method calls so the deleted worker's process is
killed and its GPU claims are released rather than touching the
currently-selected worker.
In `@tui/src/event.rs`:
- Around line 31-32: The comment above the event reader loop currently uses
"Safety:" but this is safe code; update the doc comment to use "INVARIANT:"
instead to follow repository conventions—locate the comment immediately
preceding the #[expect(clippy::disallowed_methods)] attribute and the
fire-and-forget event reader loop and replace "Safety:" with "INVARIANT:" so the
assumption is documented correctly for the safe code path.
In `@tui/src/main.rs`:
- Around line 108-116: The readiness timeout is set to 30s but the docs state
120s; update the timeout in the loop where deadline is computed (the
tokio::time::Instant::now() + tokio::time::Duration::from_secs(30) assignment
that defines deadline) to 120 seconds (or replace the literal with a named
constant like READYNESS_TIMEOUT_SECS used by the readiness polling logic) so the
loop waits up to 120s before warning and breaking.
- Around line 208-211: The current extract_port() uses rsplit(':') and fails on
URLs with paths (e.g. "http://host:31000/health"); replace that ad-hoc parsing
with a proper URL parse: call url::Url::parse(url) (or prepend "http://" if
scheme may be missing), then return the port via url.port().map(|p| p) or
url.port_or_known_default() wrapped as Option<u16>, and handle parse errors by
returning None; update the extract_port function to use url::Url parsing and
error-safe extraction instead of rsplit.
In `@tui/src/ui/detail.rs`:
- Around line 32-34: The comment above the RwLock read should use the repository
convention INVARIANT: instead of Safety: because this is documenting an
assumption in safe code; update the doc comment that precedes
#[expect(clippy::unwrap_used)] and the line let state =
app.state.read().unwrap() to start with "INVARIANT:" (e.g., "INVARIANT: RwLock
is not poisoned — no panics while holding the lock") so it matches the project's
guidance for non-unsafe code.
In `@tui/src/ui/workers.rs`:
- Around line 345-356: The code only checks worker_url prefixes "grpc://" and
"https://" for Prometheus RPS fallback, causing plain "http://" workers to
return ("0","0.0%"); update the logic that builds the per-worker load tuple (the
block using worker_url and worker_rps) to also handle "http://" (either add an
if branch for worker_url.starts_with("http://") mirroring the grpc/https
branches or collapse into a single branch that checks any scheme and uses
worker_rps.get(worker_url).copied().unwrap_or(0.0) to format "{rps:.1} r/s" and
"N/A"), referencing the existing worker_url and worker_rps variables to locate
and change the code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a61bbe20-d95f-4d53-b552-03419d395848
📒 Files selected for processing (14)
tui/src/app.rstui/src/event.rstui/src/main.rstui/src/state.rstui/src/ui/action_menu.rstui/src/ui/detail.rstui/src/ui/footer.rstui/src/ui/help.rstui/src/ui/logs.rstui/src/ui/models.rstui/src/ui/pulse.rstui/src/ui/stats_bar.rstui/src/ui/tabs.rstui/src/ui/workers.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d4bf24818f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (8)
tui/src/ui/pulse.rs (2)
379-391:⚠️ Potential issue | 🟠 MajorRead request-rate history in the compact throughput view.
Lines 379-387 still pull from
throughput_historywhile the label saysreq/s, and Lines 379-384 report “No data” even whenrequests_per_sec_historyhas samples. Narrow and 80–99 column layouts will show the wrong metric.Suggested fix
- if state.throughput_history.is_empty() { + if state.requests_per_sec_history.is_empty() { f.render_widget( Paragraph::new(Line::styled("No data", theme::label())), inner, ); return; } - let latest = state.throughput_history.back().copied().unwrap_or(0.0); + let latest = state.requests_per_sec_history.back().copied().unwrap_or(0.0); f.render_widget( Paragraph::new(Line::from(vec![ Span::styled("Latest: ", theme::label()), Span::styled(format!("{latest:.1} req/s"), theme::text().fg(theme::GREEN)), ])),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tui/src/ui/pulse.rs` around lines 379 - 391, The compact throughput view is reading from state.throughput_history but should use state.requests_per_sec_history; update the empty-check and the latest-value extraction in the block that renders the "Latest: ... req/s" paragraph to use requests_per_sec_history (replace uses of throughput_history.back()/is_empty() with requests_per_sec_history.back()/is_empty()), so the label and displayed metric match the actual request-rate history shown by the compact view.
248-250:⚠️ Potential issue | 🟡 MinorAvoid byte slicing in
shorten_gpu_name().Line 250 slices a UTF-8 string by byte index, so a non-ASCII GPU name can panic the render path. This is low-probability with current NVIDIA names, but it is still a runtime panic in a hot UI path.
Suggested fix
// Truncate if too long - if name.len() > 12 { - name[..12].to_string() + if name.chars().count() > 12 { + name.chars().take(12).collect() } else { name.to_string() } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tui/src/ui/pulse.rs` around lines 248 - 250, The function shorten_gpu_name() currently truncates the input string by byte slicing (name[..12].to_string()), which can panic on UTF-8 multi-byte characters; replace that byte-slice logic with a UTF-8 safe truncation: iterate over name.chars() and take the first N characters (or use the unicode-segmentation crate and take grapheme clusters if you need to preserve visible characters) and collect into a String so the function returns a safely truncated GPU name without panicking.tui/src/ui/workers.rs (1)
345-355:⚠️ Potential issue | 🟠 MajorUse the existing Prometheus fallback for plain
http://workers too.If
/get_loadsis missing, Line 355 falls back to hard-coded zeros for plainhttp://URLs instead of theworker_rpsmap already passed in. That makes locally spawned HTTP workers look idle whenever the load endpoint is unavailable.Suggested fix
- // gRPC/local workers: show req/s from Prometheus per-worker counts - if worker_url.starts_with("grpc://") { - let rps = worker_rps.get(worker_url).copied().unwrap_or(0.0); - return (format!("{rps:.1} r/s"), "N/A".to_string()); - } - // External workers - if worker_url.starts_with("https://") { + // Fallback: show req/s from Prometheus when /get_loads is unavailable + if worker_url.starts_with("grpc://") + || worker_url.starts_with("http://") + || worker_url.starts_with("https://") + { let rps = worker_rps.get(worker_url).copied().unwrap_or(0.0); return (format!("{rps:.1} r/s"), "N/A".to_string()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tui/src/ui/workers.rs` around lines 345 - 355, The code currently returns hard-coded zeros for plain http:// workers; update the branch handling so that URLs starting with "http://" use the same Prometheus fallback as the grpc and https branches: read rps from the worker_rps map (worker_rps.get(worker_url).copied().unwrap_or(0.0)), format it as "{rps:.1} r/s" and return "N/A" for the second value, replacing the final ("0","0.0%") result for http:// cases; locate the block using the variables worker_url and worker_rps in workers.rs and add a starts_with("http://") branch or broaden the existing condition accordingly.tui/src/main.rs (1)
208-210:⚠️ Potential issue | 🟠 MajorParse the port from the URL instead of splitting on
:.Line 210 misreads URLs with paths like
http://localhost:31000/health, so auto-start falls back to 30000/29000 and can launch on the wrong socket.Suggested fix
/// Extract port from a URL like "http://localhost:30000". fn extract_port(url: &str) -> Option<u16> { - url.rsplit(':').next()?.trim_end_matches('/').parse().ok() + reqwest::Url::parse(url).ok()?.port() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tui/src/main.rs` around lines 208 - 210, The extract_port function incorrectly splits on ':' and mis-parses URLs with paths (e.g., "http://localhost:31000/health"); update extract_port to parse the input as a URL and return the explicit port component instead of string-splitting: call Url::parse(url) (from the url crate) and use Url::port() (or port_or_known_default() if you want defaults) to extract the u16 port, handling parse errors by returning None; update the function signature/existing callers in extract_port to reflect this change.tui/src/ui/logs.rs (1)
167-178:⚠️ Potential issue | 🟠 MajorMake tail reads UTF-8-safe.
Line 172 can seek into the middle of a multibyte character. When that happens, Line 175 clears the whole buffer on decode failure, so the Logs tab goes blank; even successful mid-file reads can start with a partial first line.
Suggested fix
Ok(mut file) => { // Read at most the last 64KB to avoid blocking on large files const TAIL_BYTES: u64 = 64 * 1024; let len = file.metadata().map(|m| m.len()).unwrap_or(0); + let did_seek = len > TAIL_BYTES; if len > TAIL_BYTES { let _ = file.seek(SeekFrom::Start(len - TAIL_BYTES)); } - let mut buf = String::new(); - if file.read_to_string(&mut buf).is_err() { + let mut buf = Vec::new(); + if file.read_to_end(&mut buf).is_err() { buf.clear(); } - buf + let text = String::from_utf8_lossy(&buf).into_owned(); + if did_seek { + text.split_once('\n') + .map(|(_, rest)| rest.to_string()) + .unwrap_or_default() + } else { + text + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tui/src/ui/logs.rs` around lines 167 - 178, The tail read can seek into the middle of a UTF-8 multibyte sequence and then drop the entire buffer on decode failure; fix by reading raw bytes instead of directly into a String and then decode safely: change the logic around file.read_to_string to read_to_end into a Vec<u8> (keep TAIL_BYTES and the seek logic), then convert that byte buffer to a String using String::from_utf8_lossy or std::str::from_utf8 with a fallback so partial leading bytes don't cause a full clear—reference the variables/operations file, TAIL_BYTES, the seek(SeekFrom::Start(...)) call, and the read_to_* call when making the change.tui/src/app.rs (3)
451-460:⚠️ Potential issue | 🟠 MajorMake
:deletereuse the same cleanup path as the confirm dialog.This branch still only deletes the gateway record and drops
claimed_gpusfor the currently selected row.:delete <other-id>can therefore release the wrong claim, and locally spawned backends inworker_childrenkeep running. Route the command through the same helper used byhandle_delete_confirm(), or resolve the worker URL/process by the providedidbefore cleaning up.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tui/src/app.rs` around lines 451 - 460, The current ":delete" branch calls self.client.delete_worker(&id) and only removes claimed_gpus for the currently selected row, which can release the wrong GPU and leave worker_children running; fix by reusing the same cleanup path as handle_delete_confirm(): after successful delete_worker(&id).await, resolve the worker URL and any spawned child process corresponding to that id (use the same lookup logic used in handle_delete_confirm() to map id → worker_url and id → worker_children entry), remove the correct entry from claimed_gpus and stop/cleanup the matching worker_children, and call set_status with the id; alternatively, invoke the same helper method that handle_delete_confirm() uses to perform the cleanup instead of directly mutating claimed_gpus/worker_children.
403-408:⚠️ Potential issue | 🟠 MajorUse one shared filtered-worker view for selection and actions.
selected_worker(),start_delete(), andclamp_selection()still disagree about what "visible workers" means: one matchesruntime_type, one only matchesid/url, and one ignores the filter entirely. Under an active filter this can leaveselected_indexout of range or makedresolve a different/no worker than the other actions. Extract one helper for the filtered list and reuse it everywhere, then re-clamp immediately after updatingactive_filter.Also applies to: 807-825, 1126-1145, 1360-1368
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tui/src/app.rs` around lines 403 - 408, The codebase has inconsistent definitions of "visible workers" across selected_worker(), start_delete(), and clamp_selection(), causing out-of-range or wrong selections when active_filter changes; extract a single helper (e.g., a method like filtered_workers() or get_visible_workers()) that returns the list of workers filtered by the same criteria (use runtime_type + id/url + active_filter logic you want) and replace all ad-hoc filtering in selected_worker(), start_delete(), clamp_selection(), and the other occurrences (around the noted ranges) to use that helper; after updating active_filter (when you set self.active_filter based on self.input_buffer and change self.input_mode) call clamp_selection() immediately to re-clamp selected_index against the unified filtered list so selection remains consistent.
540-559:⚠️ Potential issue | 🟠 MajorToggle health checks from the config flag, not
is_healthy.An actually unhealthy worker with health checks still enabled takes the
disable = falsebranch here, so the "toggle" becomes a no-op and the TUI can never disable checks on that worker. Derive the next value from the worker's currentdisable_health_checksetting instead of inferring it from liveness.Also applies to: 918-944
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tui/src/app.rs` around lines 540 - 559, The toggle currently uses worker.is_healthy; instead derive the next disable value from the worker's current health.disable_health_check flag. Retrieve current_disabled via worker.health.as_ref().and_then(|h| h.disable_health_check).unwrap_or(false) and set disable = !current_disabled, then build the openai_protocol::worker::WorkerUpdateRequest::health with that disable; apply the same change to the other occurrence around the WorkerUpdateRequest construction (the similar block mentioned at lines 918-944).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tui/src/main.rs`:
- Around line 97-101: The spawned gateway process may survive early returns or
panics because the Child in _gateway_child isn't guaranteed to be killed; update
the Command invocation in main.rs (the tokio::process::Command::new("smg") chain
that creates child and assigns to _gateway_child) to call .kill_on_drop(true)
before .spawn() so the Child will be terminated automatically when dropped,
ensuring cleanup on all code paths including early returns and panic hook paths
that restore terminal state but don't explicitly kill the process.
In `@tui/src/state.rs`:
- Around line 412-448: The rate calculations (rps and token/s) in the metrics
handling (functions/variables: parse_request_count, parse_token_counts,
s.prev_request_count, s.prev_input_tokens, s.prev_output_tokens) incorrectly
assume each successful scrape is exactly interval_secs apart and produce spikes
when a scrape failed; fix by tracking the timestamp of the last successful
metrics scrape (add e.g. s.last_metrics_ts) and, when computing deltas for
rps/in_tps/out_tps, divide by the actual elapsed seconds between current scrape
and s.last_metrics_ts (or if you prefer simpler behavior, clear s.prev_*
counters when fetch_metrics() fails so you don't compute a rate on a
multi-interval gap); update both the request and token rate calculations (also
the worker_rps logic referenced at 477-486) to use the elapsed time and set
s.last_metrics_ts = now on successful parses.
---
Duplicate comments:
In `@tui/src/app.rs`:
- Around line 451-460: The current ":delete" branch calls
self.client.delete_worker(&id) and only removes claimed_gpus for the currently
selected row, which can release the wrong GPU and leave worker_children running;
fix by reusing the same cleanup path as handle_delete_confirm(): after
successful delete_worker(&id).await, resolve the worker URL and any spawned
child process corresponding to that id (use the same lookup logic used in
handle_delete_confirm() to map id → worker_url and id → worker_children entry),
remove the correct entry from claimed_gpus and stop/cleanup the matching
worker_children, and call set_status with the id; alternatively, invoke the same
helper method that handle_delete_confirm() uses to perform the cleanup instead
of directly mutating claimed_gpus/worker_children.
- Around line 403-408: The codebase has inconsistent definitions of "visible
workers" across selected_worker(), start_delete(), and clamp_selection(),
causing out-of-range or wrong selections when active_filter changes; extract a
single helper (e.g., a method like filtered_workers() or get_visible_workers())
that returns the list of workers filtered by the same criteria (use runtime_type
+ id/url + active_filter logic you want) and replace all ad-hoc filtering in
selected_worker(), start_delete(), clamp_selection(), and the other occurrences
(around the noted ranges) to use that helper; after updating active_filter (when
you set self.active_filter based on self.input_buffer and change
self.input_mode) call clamp_selection() immediately to re-clamp selected_index
against the unified filtered list so selection remains consistent.
- Around line 540-559: The toggle currently uses worker.is_healthy; instead
derive the next disable value from the worker's current
health.disable_health_check flag. Retrieve current_disabled via
worker.health.as_ref().and_then(|h| h.disable_health_check).unwrap_or(false) and
set disable = !current_disabled, then build the
openai_protocol::worker::WorkerUpdateRequest::health with that disable; apply
the same change to the other occurrence around the WorkerUpdateRequest
construction (the similar block mentioned at lines 918-944).
In `@tui/src/main.rs`:
- Around line 208-210: The extract_port function incorrectly splits on ':' and
mis-parses URLs with paths (e.g., "http://localhost:31000/health"); update
extract_port to parse the input as a URL and return the explicit port component
instead of string-splitting: call Url::parse(url) (from the url crate) and use
Url::port() (or port_or_known_default() if you want defaults) to extract the u16
port, handling parse errors by returning None; update the function
signature/existing callers in extract_port to reflect this change.
In `@tui/src/ui/logs.rs`:
- Around line 167-178: The tail read can seek into the middle of a UTF-8
multibyte sequence and then drop the entire buffer on decode failure; fix by
reading raw bytes instead of directly into a String and then decode safely:
change the logic around file.read_to_string to read_to_end into a Vec<u8> (keep
TAIL_BYTES and the seek logic), then convert that byte buffer to a String using
String::from_utf8_lossy or std::str::from_utf8 with a fallback so partial
leading bytes don't cause a full clear—reference the variables/operations file,
TAIL_BYTES, the seek(SeekFrom::Start(...)) call, and the read_to_* call when
making the change.
In `@tui/src/ui/pulse.rs`:
- Around line 379-391: The compact throughput view is reading from
state.throughput_history but should use state.requests_per_sec_history; update
the empty-check and the latest-value extraction in the block that renders the
"Latest: ... req/s" paragraph to use requests_per_sec_history (replace uses of
throughput_history.back()/is_empty() with
requests_per_sec_history.back()/is_empty()), so the label and displayed metric
match the actual request-rate history shown by the compact view.
- Around line 248-250: The function shorten_gpu_name() currently truncates the
input string by byte slicing (name[..12].to_string()), which can panic on UTF-8
multi-byte characters; replace that byte-slice logic with a UTF-8 safe
truncation: iterate over name.chars() and take the first N characters (or use
the unicode-segmentation crate and take grapheme clusters if you need to
preserve visible characters) and collect into a String so the function returns a
safely truncated GPU name without panicking.
In `@tui/src/ui/workers.rs`:
- Around line 345-355: The code currently returns hard-coded zeros for plain
http:// workers; update the branch handling so that URLs starting with "http://"
use the same Prometheus fallback as the grpc and https branches: read rps from
the worker_rps map (worker_rps.get(worker_url).copied().unwrap_or(0.0)), format
it as "{rps:.1} r/s" and return "N/A" for the second value, replacing the final
("0","0.0%") result for http:// cases; locate the block using the variables
worker_url and worker_rps in workers.rs and add a starts_with("http://") branch
or broaden the existing condition accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 598d00ab-6a02-4131-945d-621aa77ce91a
📒 Files selected for processing (11)
tui/src/app.rstui/src/main.rstui/src/state.rstui/src/ui/footer.rstui/src/ui/help.rstui/src/ui/logs.rstui/src/ui/models.rstui/src/ui/pulse.rstui/src/ui/stats_bar.rstui/src/ui/tabs.rstui/src/ui/workers.rs
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
2 similar comments
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
Full-featured terminal dashboard with: - Pulse: Worker health, throughput sparkline, request stats - Workers: Table with running reqs, token usage, detail panel - Chat: Streaming chat with markdown, multi-turn support - Logs: Sub-tabs for TUI, gateway, and per-worker logs - Stats bar: Workers, Circuit Breakers, REQ/S, AVG LATENCY - Worker management: External providers, local sglang/vllm with GPU selection - Gateway auto-start with IGW + round_robin Signed-off-by: key4ng <rukeyang@gmail.com>
Signed-off-by: key4ng <rukeyang@gmail.com>
- Filter KeyEventKind::Press to prevent duplicate key handling - Restore terminal state on setup errors (RAII-style cleanup) - Return JoinHandle from spawn_poller for graceful shutdown - Fix duplicate throughput_history push (use only Prometheus rps) - Guard negative latency on Prometheus counter reset - Derive connection state from liveness, not readiness - Use check_alive instead of check_health for auto-start detection - Fix Box::leak per-frame in SelectModel menu rendering - Use circuit breaker state from Prometheus instead of worker health - Fix UTF-8 panic in truncate_str (char-based truncation) - Validate runtime type parse with user feedback instead of silent default - Toggle health check actually toggles (reads current state) - Don't block 'q' key in chat when input buffer has content - Don't hard-code external chat models to gpt-5.4 prefix - Roll back spawned worker on gateway registration failure - Clean up GPU claims in :delete command - Fix detail pane using clamped selection index - Aggregate worker loads across all TP entries - Update footer hints to show 1-7 views - Fix duplicate "Navigation" header in help text - Fix Models table "OWNER" header to "NAME" - Use view.index() in tabs for consistency - Add Request Stats to narrow pulse layout - Handle zero-worker case in stats bar - Limit file I/O in log render to last 64KB Signed-off-by: key4ng <rukeyang@gmail.com>
- Implemented a distinction between quitting the TUI and performing a full shutdown (Ctrl+C×2). - Updated README and help text to clarify new shutdown commands. - Adjusted model filtering to limit OpenAI models displayed to gpt-5.4* for better manageability. - Improved handling of worker processes during shutdown to ensure proper cleanup. Signed-off-by: key4ng <rukeyang@gmail.com>
- Fix extract_port() to handle URLs with paths - Increase auto-start readiness timeout from 30s to 120s - Clamp gauge_bar ratio to [0.0, 1.0] - Fix compact throughput reading wrong metric (use requests_per_sec_history) - Fix shorten_gpu_name UTF-8 boundary issue - Clamp selection against filtered list, not full worker list - Surface mid-stream SSE error events in chat - Fix logs seek UTF-8 boundary (skip partial first line) - Fix :delete command to look up worker URL by ID from state - Filter chat models to gpt-5.4* for OpenAI only - q quits TUI only, Ctrl+C×2 for full shutdown - Update README and help text for new exit behavior Signed-off-by: key4ng <rukeyang@gmail.com>
- Updated shutdown behavior to ensure worker processes begin terminating during terminal restoration. - Increased the time window for recognizing a second Ctrl+C from 2 seconds to 3 seconds for full shutdown. - Added functionality to kill processes listening on a specified TCP port if the auto-started gateway is not present. - Improved logging messages for clarity during shutdown operations. Signed-off-by: key4ng <rukeyang@gmail.com>
- Add multi-field custom model form (model ID, TP, extra args) with Tab/Shift+Tab field navigation - Extra args passed to worker launch command (e.g. --max-model-len) - Fix logs j/k scroll: j no longer no-ops at auto-scroll bottom - Change worker log cycle key from c to w to avoid conflict - Update footer, help text, and README for new keybindings Signed-off-by: key4ng <rukeyang@gmail.com>
- Modified the GPU query to check for <10% VRAM utilization instead of >2GB free memory. - Updated the command arguments to include both used and total memory metrics. - Adjusted the filtering logic to ensure only GPUs meeting the new criteria are returned. Signed-off-by: key4ng <rukeyang@gmail.com> Signed-off-by: Keyang Ru <rukeyang@gmail.com>
key4ng
force-pushed
the
keyang/tui-redesign
branch
from
March 26, 2026 19:16
0323e8d to
cad6309
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cad6309ea5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+773
to
+776
| if self.chat_streaming { | ||
| self.chat_streaming = false; | ||
| self.chat_stream_rx = None; | ||
| if let Some(msg) = self.chat_messages.last_mut() { |
There was a problem hiding this comment.
This path marks streaming as stopped in the UI but does not cancel the spawned /v1/chat//v1/responses task, so pressing Esc during a long response still lets the upstream request run to completion (token/cost and backend load continue in the background). Because tx.send(...) errors are ignored in the stream task, dropping chat_stream_rx does not actually stop generation; store the JoinHandle (or a cancellation token) and abort on Esc.
Useful? React with 👍 / 👎.
| // Safety: fire-and-forget background poller that runs for the app's lifetime | ||
| #[expect(clippy::disallowed_methods)] | ||
| tokio::spawn(async move { | ||
| let mut ticker = tokio::time::interval(std::time::Duration::from_secs(interval_secs)); |
There was a problem hiding this comment.
The poll period is user-controlled, and a zero value reaches Duration::from_secs(interval_secs) here, which yields an invalid polling configuration (and later rate computations divide by interval_secs). This can crash or destabilize the dashboard loop when users pass --poll-interval 0; enforce a minimum of 1 second at argument parsing or before constructing the interval.
Useful? React with 👍 / 👎.
smfirmin
pushed a commit
to smfirmin/smg
that referenced
this pull request
Apr 2, 2026
Signed-off-by: key4ng <rukeyang@gmail.com>
2 of 4 tasks
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
1 participant
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.
Description
Problem
SMG lacks a visual interface for monitoring workers, managing routing, and interacting with models. Users rely on curl commands and raw Prometheus metrics to understand gateway state, which is slow and error-prone — especially when managing multiple local and external workers across GPUs.
Solution
A full-featured terminal UI (
smg-tui) built with Ratatui that connects to a running SMG gateway and provides real-time monitoring, worker management, and an interactive chat playground — all from the terminal.Changes
Views (7 tabs)
Stats Bar
Four metric cards: Workers (count + health), Circuit Breakers (open/closed from Prometheus), REQ/S (from
smg_router_requests_total+ in-flight count), AVG LATENCY (fromsmg_router_request_durationwith gauge bar)Worker Management
nvidia-smiwithCUDA_VISIBLE_DEVICES, GPU claim tracking to prevent double-allocationGateway Auto-Start
--auto-startlaunchessmg launch --enable-igw --policy round_robin, polls health endpoint, kills on exitChat Playground
previous_response_idMetrics Integration
Polls Prometheus for: req/s, avg latency, circuit breaker state, per-worker request counts, active connections, in-flight requests, token counts
Test Plan
cargo check -p smg-tui— compiles with zero warnings--auto-start, verify gateway starts with IGW + round_robinSummary by CodeRabbit
New Features
Documentation
Chores