feat(workers-dev): persistent engine connection + TUI overhaul - #368
Merged
Conversation
…rkers The TUI polled engine status by spawning `iii trigger engine::workers::list` every poll tick; each spawn opened a one-shot WebSocket the engine logged as a worker register/unregister pair, flooding engine logs every ~2s. Replace it with one persistent iii-sdk connection (labeled `workers-dev`), lazily opened and reused for every poll, so the engine sees one register at connect and one unregister at clean exit. Also fixes several issues found while reviewing the worker: - Run each worker in its own process group and group-kill (SIGTERM then SIGKILL) on stop. `cargo run` forks the worker binary as a child, so killing only cargo orphaned the worker (it kept running, still connected). This also repairs --stop-on-exit, which was silently broken. - Poll the engine off the UI thread via a watch channel so a slow or unreachable engine can't freeze keyboard input; shorten the query timeout to 3s. - Surface "engine unreachable" in the TUI header and `status` output instead of silently rendering every worker as disconnected. - Serialize TUI start/stop/restart via an in-flight counter so fast keypresses can't interleave actions on the same worker. - `workers-dev up` now boots `iii -c harness/engine.config.yaml` (detached) when the engine isn't reachable.
…olish Reworks the dashboard TUI and fixes DX gaps found dogfooding it: - Scrollable in-place log pane with a follow toggle (PgUp/PgDn, f); no longer ejects the dashboard to tail logs. - At-a-glance health summary in the header (connected/compiling/crashed/stopped) and a `?` overlay with the full key reference. - Filter workers by name with `/` (group headers kept only for matching groups). - Inline crash exit codes in the worker table. - Event-driven rendering: redraw only on input, a new snapshot, a spinner tick, or a live-log refresh — idle no longer repaints continuously. - Animated spinner for compiling workers; resizable log/table split (+/-). - Restart confirm dialog lists the dependents that will also restart. - Worker-action failures now surface as a footer banner instead of eprintln, which was lost or garbled under the alt-screen. - Repaint on terminal resize (the event-driven loop had ignored resize events). Drops the per-worker log tail from the poll snapshot (logs are fetched on demand for the selected worker), making each status poll cheaper.
- Cap the log pane so the worker list (primary content) stays visible: on short terminals it no longer starves the list to ~2 rows while the log pane sits empty; the table flexes to fill on tall terminals. - Footer adapts to width in three tiers, always keeping `? keys · q quit`, which previously truncated off at narrow widths. - Content-fit column widths so Process/Engine/PID/Uptime align in a tight cluster instead of floating across the row behind a wide Uptime column. - "No workers match …" placeholder when a filter yields nothing. - Status-glyph legend (connected/compiling/crashed/stopped) in the ? overlay. - Drop the duplicated "workers-dev" header box title.
The Worker column was 38 wide only to hold two things stuffed into the name cell, which pushed every worker's status far to the right. Move both into the Process column where management/process state belongs: - Non-startable workers show "external" (was a repeated "(iii worker add)" suffix on the name). This also resolves the "Process: stopped / Engine: connected" contradiction — a connected unmanaged worker now reads "external / connected". - A crash shows "exit N" in the Process column (was trailing the name). The name cell is now just glyph + name, so the Worker column shrinks to 24 and the status block sits right next to the name. At narrow widths all five columns now fit. The ? overlay gains an "external" legend line.
Rework the dashboard into a two-column layout on wide terminals: the worker list (content-fit at 66 cols) on the left, the selected worker's logs flexing to fill the rest on the right. Below ~103 cols the two panes stack vertically as before, so neither is crushed. Header and footer span full width; modals re-anchor to the body so they keep their width in either layout. Critique-driven polish on top of the rework: - Process column shows "elsewhere" when a worker is engine-connected but not spawned here, instead of the contradictory "stopped" + "connected". - +/- drags the column divider in two-column mode (50..66) and still resizes the log height when stacked. - 1-col gutter between panes so their borders no longer fuse into a seam. - Muted text uses ANSI gray (7) instead of bright-black (8), which themes define with readable contrast.
Muted styles now apply Modifier::DIM to the default foreground instead of a fixed ANSI gray, so they de-emphasize correctly on both light- and dark-background terminals. A single canonical muted_cell_style() backs the rest (footer, hints, "external"/"elsewhere", "—", stopped, Other header).
…k::runtime WorkerMetadata and IIIConnectionState were imported via the legacy iii_sdk::iii re-export; 0.20 canonicalizes them under iii_sdk::runtime (where all sibling workers already import them). No behavior change.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
skill-check — worker0 verified, 29 skipped (no docs/).
Four for four. Nicely done. |
|
Warning Review limit reached
Next review available in: 18 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughIntroduces a persistent Changesworkers-dev: persistent engine client, process-group lifecycle, and TUI overhaul
Sequence Diagram(s)sequenceDiagram
participant main
participant Orchestrator
participant IIIClient
participant Engine
participant TUI
main->>Orchestrator: new(config, progress=true)
Orchestrator->>IIIClient: OnceCell init (lazy)
main->>Orchestrator: ensure_engine()
Orchestrator->>IIIClient: engine_client() → register + wait Connected
IIIClient->>Engine: WebSocket connect
Engine-->>IIIClient: Connected
main->>TUI: run()
loop background poller
Orchestrator->>IIIClient: trigger engine::workers::list
IIIClient->>Engine: TriggerRequest
Engine-->>IIIClient: EngineWorkersResponse
Orchestrator-->>TUI: watch channel DashboardState{views, engine_error}
end
TUI->>Orchestrator: spawn_start / spawn_stop / spawn_restart
Orchestrator->>IIIClient: start_one() process_group(0)
Orchestrator->>IIIClient: terminate_process_group() SIGTERM→SIGKILL
main->>Orchestrator: shutdown()
Orchestrator->>IIIClient: close connection
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
workers-dev/src/status.rs (1)
71-87: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
exit_codeis still invisible inworkers-dev status.
build_view()now fillsWorkerView.exit_code, but this formatter still prints only process/engine/PID/uptime. On the CLI status path, crashed workers therefore still lose the exit-code detail this field was added to expose inline.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@workers-dev/src/status.rs` around lines 71 - 87, The workers-dev status formatter in build_view()/the status printing block is still omitting the new exit_code field, so crashed worker details are not shown inline. Update the println! output for WorkerView to include exit_code alongside the existing WORKER, PROCESS, ENGINE, PID, and UPTIME columns, and adjust the header/format string in the same status rendering path so the value from WorkerView.exit_code is visible when present.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@workers-dev/README.md`:
- Around line 38-39: The README text in the `workers-dev` section is
inconsistent with the current TUI status labels from `process_cell()`. Update
the sentence describing workers connected to the engine so it uses the same
`Process: external` wording as the rest of the section, and make sure the
description for non-Rust workers installed via `iii worker add` matches the new
`external` label everywhere.
In `@workers-dev/src/config.rs`:
- Around line 71-78: In Config::load, the engine URL canonicalization is always
rebuilding the URL as ws:// even when parse_engine_url() parsed a secure wss://
endpoint. Preserve the original scheme when reconstructing engine_url by
carrying the parsed scheme through parse_engine_url() (or equivalent helper) and
using it in the final format so CLI/config TLS endpoints remain wss:// after
load.
In `@workers-dev/src/discover.rs`:
- Around line 77-82: The skip-on-mismatch behavior in discover logic is
incomplete because explicit worker loading still hard-fails later. Update the
worker loading path in WorkerGraph::load() and/or Config::load() so a stale
workers entry whose iii.worker.yaml name does not match the folder is handled
consistently with discover.rs: emit the mismatch warning and skip that worker
instead of aborting startup. Keep the check aligned with the existing
name-versus-folder comparison used in the discovery code so both auto-discovered
and explicitly listed workers follow the same “skip and continue” behavior.
In `@workers-dev/src/logs.rs`:
- Around line 75-86: The timestamp rendering in
`split_tracing_timestamp`/`Line::from` is allowing one extra visible column
because the separator `" "` is always added even when `truncate_chars(ts,
max_width)` already fills the width. Update this branch so the separator and
remaining text are only appended when there is space left, and compute the
remaining width from the actual rendered timestamp length to keep the total
within `max_width`.
In `@workers-dev/src/orchestrator.rs`:
- Around line 324-365: The wait_connected method is masking engine polling
failures by turning engine_workers() errors into an empty list, which makes
start/restart wait until timeout and misreport the worker as the problem. Update
orchestrator::wait_connected to preserve and propagate the last engine query
error instead of defaulting to no workers, and make the timeout/bail path
mention that engine polling failed if that is what happened. Keep the existing
connected and crashed/stopped checks intact, but ensure the shared client/query
failure from engine_workers() is surfaced clearly.
In `@workers-dev/src/tui/mod.rs`:
- Around line 446-450: The KeyCode::Char('x') handler in
workers-dev/src/tui/mod.rs currently allows stopping any row with a worker_name,
including external and elsewhere entries that have no local process to
terminate. Update the x-action handling in the TUI event logic to gate stop
behavior on rows that are actually owned and spawned by workers-dev, and keep
the action disabled or ignored for external/elsewhere rows. Use the existing
UiMode, worker_name, and spawn_stop flow to locate the check and make the stop
path conditional on the row’s ownership state.
- Around line 906-910: The empty-state text in the `total == 0` branch of
`mod.rs` is using the wrong message when `selected_name` is `None`. Update the
logic around this `lines: Vec<Line>` construction to distinguish between
“selected worker with no output” and “no worker selected because the filter
removed everything,” and render a different hint for the no-selection case. Keep
the existing selected-worker message only when `selected_name` is present.
---
Outside diff comments:
In `@workers-dev/src/status.rs`:
- Around line 71-87: The workers-dev status formatter in build_view()/the status
printing block is still omitting the new exit_code field, so crashed worker
details are not shown inline. Update the println! output for WorkerView to
include exit_code alongside the existing WORKER, PROCESS, ENGINE, PID, and
UPTIME columns, and adjust the header/format string in the same status rendering
path so the value from WorkerView.exit_code is visible when present.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8822cc69-1734-4b89-ba6c-68db4c7e213b
⛔ Files ignored due to path filters (1)
workers-dev/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
workers-dev/Cargo.tomlworkers-dev/README.mdworkers-dev/src/commands/mod.rsworkers-dev/src/config.rsworkers-dev/src/discover.rsworkers-dev/src/graph.rsworkers-dev/src/logs.rsworkers-dev/src/main.rsworkers-dev/src/orchestrator.rsworkers-dev/src/runtime.rsworkers-dev/src/status.rsworkers-dev/src/tui/mod.rsworkers-dev/src/tui/theme.rs
- config: preserve wss:// scheme when canonicalizing engine_url - config: skip (warn) configured workers missing/mismatched in discovery instead of aborting startup later in WorkerGraph::load - logs: stop a max-width timestamp overflowing the log pane by one column - orchestrator: surface engine-query failures in wait_connected timeout - tui: only allow stop on workers with a live local process - tui: distinct empty-log hint when no worker is selected - README: use the "Process: external" label the TUI actually renders
ytallo
approved these changes
Jun 29, 2026
This was referenced Jul 3, 2026
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
2 participants
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.
Builds on the dev TUI introduced in #311. All changes are isolated to
workers-dev/— no other crate is touched.Engine connection & process lifecycle
iii triggersubprocess with one persistentIIIClient(lazyOnceCell, reused for every poll). This stops the engine register/unregister storm that fired every tick.cargo runno longer orphans the worker binary it forked.TUI overhaul
+/-drags the divider (or resizes log height when stacked).▶ live/⏸ scrolledfollow state; name filter (/); at-a-glance health summary (●◐✗○) and⚠ unreachableengine banner.watch, action errors surfaced in a footer banner).Polish
external(installed viaiii worker add) andelsewhere(engine-connected but started outside this tool) instead of a contradictorystopped+connected.Modifier::DIMon the default foreground (readable on both light and dark terminals).SDK
WorkerMetadata/IIIConnectionStatefrom the canonicaliii_sdk::runtime(matching all sibling workers), not the legacyiii_sdk::iiire-export.Builds clean (
cargo build,clippy— no issues,cargo fmt --check),cargo test— 14 passed. Design-critique trend across iterations: 29 → 31 → 32 → 33 / 40.Summary by CodeRabbit
New Features
Bug Fixes
Documentation