Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion src/cli/commands/meta.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,9 @@ use crate::cli::action::{GlobalOpts, run_action};
use crate::cli::init_flow::{
InitCommandOptions, InitPlan, InitResolutionContext, resolve_init_plan, run_init_wizard,
};
use crate::cli::opentui::{launch_opentui, should_launch_opentui};
use crate::cli::opentui::{
launch_opentui, launch_opentui_watch, should_launch_opentui, should_launch_opentui_watch,
};
use crate::cli::parsers::{as_optional_string, parse_positive_int, parse_status_csv};
use crate::cli::render::{print_history, print_orphans_result, print_repair_result};
use crate::cli::tui::{TuiOptions, TuiView, start_tui};
Expand Down Expand Up @@ -326,6 +328,15 @@ pub fn execute_watch(service: &TasqueService, args: WatchArgs, opts: GlobalOpts)
return error.exit_code;
}
};
if should_launch_opentui_watch(&watch_options) {
match launch_opentui_watch(&watch_options) {
Ok(exit_code) => return exit_code,
Err(error) => {
eprintln!("WARN: {}. Falling back to built-in watch renderer.", error);
}
}
}

start_watch(service, watch_options)
}

Expand Down
121 changes: 101 additions & 20 deletions src/cli/opentui.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
use crate::app::runtime::find_tasque_root;
use crate::cli::tui::{TuiOptions, TuiView};
use crate::cli::watch::WatchOptions;
use crate::types::TaskStatus;
use std::io::IsTerminal;
use std::path::{Path, PathBuf};
use std::process::{Command, Stdio};
Expand All @@ -11,21 +13,25 @@ const BUNDLED_TUI_BINARY: &str = "tsq-tui.exe";
const BUNDLED_TUI_BINARY: &str = "tsq-tui";

pub fn should_launch_opentui(options: &TuiOptions) -> bool {
interactive_launch_ready(options.json, options.once) && launch_target_available()
}

pub fn should_launch_opentui_watch(options: &WatchOptions) -> bool {
interactive_launch_ready(options.json, options.once) && launch_target_available()
}

fn interactive_launch_ready(json: bool, once: bool) -> bool {
if std::env::var("TSQ_OPENTUI_DISABLE")
.ok()
.as_deref()
.is_some_and(|value| value == "1")
{
return false;
}
if options.json
|| options.once
|| !std::io::stdin().is_terminal()
|| !std::io::stdout().is_terminal()
{
return false;
}
!json && !once && std::io::stdin().is_terminal() && std::io::stdout().is_terminal()
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

fn launch_target_available() -> bool {
if explicit_bundled_tui_path().is_some() && resolve_bundled_tui_path().is_some() {
return true;
}
Expand All @@ -45,8 +51,47 @@ pub fn should_launch_opentui(options: &TuiOptions) -> bool {
}

pub fn launch_opentui(options: &TuiOptions) -> Result<i32, String> {
let mut command = build_launch_command()?;
apply_shared_env(&mut command);
// Clear any inherited watch mode so `tsq tui` always renders the tabbed App.
command.env_remove("TSQ_TUI_MODE");
command.env("TSQ_TUI_INTERVAL", options.interval.to_string());
command.env("TSQ_TUI_STATUS", status_csv(&options.statuses));
command.env("TSQ_TUI_VIEW", view_to_env(options.view));
apply_assignee_env(&mut command, options.assignee.as_deref());

run_command(command)
}

pub fn launch_opentui_watch(options: &WatchOptions) -> Result<i32, String> {
let mut command = build_launch_command()?;
apply_shared_env(&mut command);
command.env("TSQ_TUI_MODE", "watch");
command.env("TSQ_TUI_INTERVAL", options.interval.to_string());
command.env("TSQ_TUI_STATUS", status_csv(&options.statuses));
command.env("TSQ_WATCH_TREE", if options.tree { "1" } else { "0" });
apply_assignee_env(&mut command, options.assignee.as_deref());

run_command(command)
}

// Set the assignee filter only when requested, and otherwise clear any value
// inherited from the parent environment so a stale `TSQ_TUI_ASSIGNEE` can't
// silently filter a plain `tsq watch` / `tsq tui` run.
fn apply_assignee_env(command: &mut Command, assignee: Option<&str>) {
match assignee {
Some(value) => {
command.env("TSQ_TUI_ASSIGNEE", value);
}
None => {
command.env_remove("TSQ_TUI_ASSIGNEE");
}
}
}

fn build_launch_command() -> Result<Command, String> {
let prefer_bundle = explicit_bundled_tui_path().is_some() || explicit_entry_path().is_none();
let mut command = if let Some(bundle) = if prefer_bundle {
let command = if let Some(bundle) = if prefer_bundle {
resolve_bundled_tui_path()
} else {
None
Expand All @@ -61,23 +106,19 @@ pub fn launch_opentui(options: &TuiOptions) -> Result<i32, String> {
command.arg("run").arg(&entry);
command
};
Ok(command)
}

fn apply_shared_env(command: &mut Command) {
if let Ok(bin) = std::env::current_exe() {
command.env("TSQ_TUI_BIN", bin);
}

command.env("TSQ_TUI_INTERVAL", options.interval.to_string());
command.env("TSQ_TUI_STATUS", status_csv(options));
command.env("TSQ_TUI_VIEW", view_to_env(options.view));

if let Some(assignee) = options.assignee.as_deref() {
command.env("TSQ_TUI_ASSIGNEE", assignee);
}

if let Some(root) = find_tasque_root() {
command.current_dir(root);
}
}

fn run_command(mut command: Command) -> Result<i32, String> {
let status = command
.status()
.map_err(|error| format!("failed launching OpenTUI: {error}"))?;
Expand Down Expand Up @@ -179,9 +220,8 @@ fn view_to_env(view: TuiView) -> &'static str {
}
}

fn status_csv(options: &TuiOptions) -> String {
options
.statuses
fn status_csv(statuses: &[TaskStatus]) -> String {
statuses
.iter()
.map(|status| crate::domain::event_payload_codecs::task_status_as_str(*status))
.collect::<Vec<_>>()
Expand Down Expand Up @@ -224,6 +264,47 @@ mod tests {
assert!(!dependencies_are_available(&entry));
}

#[test]
fn interactive_gate_rejects_json_and_once() {
// The `--json` and `--once` bypass paths never launch OpenTUI, regardless
// of TTY state, so the OpenTUI app's own `watch --once --json` subprocess
// always falls through to the plain Rust path (no launch recursion).
assert!(!interactive_launch_ready(true, false));
assert!(!interactive_launch_ready(false, true));
}

#[test]
fn interactive_gate_respects_disable_env() {
unsafe {
std::env::set_var("TSQ_OPENTUI_DISABLE", "1");
}
let disabled = interactive_launch_ready(false, false);
unsafe {
std::env::remove_var("TSQ_OPENTUI_DISABLE");
}
assert!(!disabled);
}

#[test]
fn assignee_env_set_when_present() {
let mut command = Command::new("true");
apply_assignee_env(&mut command, Some("alice"));
let key = std::ffi::OsStr::new("TSQ_TUI_ASSIGNEE");
let found = command.get_envs().find(|(name, _)| *name == key);
assert_eq!(found, Some((key, Some(std::ffi::OsStr::new("alice")))));
}

#[test]
fn assignee_env_cleared_when_absent() {
let mut command = Command::new("true");
apply_assignee_env(&mut command, None);
// `env_remove` records an explicit removal (key -> None) so the child
// cannot inherit a stale TSQ_TUI_ASSIGNEE from the parent environment.
let key = std::ffi::OsStr::new("TSQ_TUI_ASSIGNEE");
let found = command.get_envs().find(|(name, _)| *name == key);
assert_eq!(found, Some((key, None)));
}

Comment on lines +267 to +307

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Env-mutating tests race under the default parallel test runner.

interactive_gate_respects_disable_env (and the pre-existing resolves_explicit_bundled_tui_binary) call std::env::set_var/remove_var on shared process env vars without synchronization. Rust's default cargo test harness runs tests concurrently across threads, so a parallel test reading TSQ_OPENTUI_DISABLE/TSQ_OPENTUI_BIN mid-mutation can observe unexpected state — this is exactly the multithreaded hazard that made set_var/remove_var unsafe in the 2024 edition. Consider serializing these tests (e.g. serial_test crate or a shared Mutex guard) to avoid CI flakiness, especially since the coding guidelines require cargo test --quiet to pass before handoff.

♻️ Example fix using a shared mutex guard
+static ENV_TEST_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(());
+
 #[test]
 fn interactive_gate_respects_disable_env() {
+    let _guard = ENV_TEST_LOCK.lock().unwrap();
     unsafe {
         std::env::set_var("TSQ_OPENTUI_DISABLE", "1");
     }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
#[test]
fn interactive_gate_rejects_json_and_once() {
// The `--json` and `--once` bypass paths never launch OpenTUI, regardless
// of TTY state, so the OpenTUI app's own `watch --once --json` subprocess
// always falls through to the plain Rust path (no launch recursion).
assert!(!interactive_launch_ready(true, false));
assert!(!interactive_launch_ready(false, true));
}
#[test]
fn interactive_gate_respects_disable_env() {
unsafe {
std::env::set_var("TSQ_OPENTUI_DISABLE", "1");
}
let disabled = interactive_launch_ready(false, false);
unsafe {
std::env::remove_var("TSQ_OPENTUI_DISABLE");
}
assert!(!disabled);
}
#[test]
fn assignee_env_set_when_present() {
let mut command = Command::new("true");
apply_assignee_env(&mut command, Some("alice"));
let key = std::ffi::OsStr::new("TSQ_TUI_ASSIGNEE");
let found = command.get_envs().find(|(name, _)| *name == key);
assert_eq!(found, Some((key, Some(std::ffi::OsStr::new("alice")))));
}
#[test]
fn assignee_env_cleared_when_absent() {
let mut command = Command::new("true");
apply_assignee_env(&mut command, None);
// `env_remove` records an explicit removal (key -> None) so the child
// cannot inherit a stale TSQ_TUI_ASSIGNEE from the parent environment.
let key = std::ffi::OsStr::new("TSQ_TUI_ASSIGNEE");
let found = command.get_envs().find(|(name, _)| *name == key);
assert_eq!(found, Some((key, None)));
}
static ENV_TEST_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(());
#[test]
fn interactive_gate_rejects_json_and_once() {
// The `--json` and `--once` bypass paths never launch OpenTUI, regardless
// of TTY state, so the OpenTUI app's own `watch --once --json` subprocess
// always falls through to the plain Rust path (no launch recursion).
assert!(!interactive_launch_ready(true, false));
assert!(!interactive_launch_ready(false, true));
}
#[test]
fn interactive_gate_respects_disable_env() {
let _guard = ENV_TEST_LOCK.lock().unwrap();
unsafe {
std::env::set_var("TSQ_OPENTUI_DISABLE", "1");
}
let disabled = interactive_launch_ready(false, false);
unsafe {
std::env::remove_var("TSQ_OPENTUI_DISABLE");
}
assert!(!disabled);
}
#[test]
fn assignee_env_set_when_present() {
let mut command = Command::new("true");
apply_assignee_env(&mut command, Some("alice"));
let key = std::ffi::OsStr::new("TSQ_TUI_ASSIGNEE");
let found = command.get_envs().find(|(name, _)| *name == key);
assert_eq!(found, Some((key, Some(std::ffi::OsStr::new("alice")))));
}
#[test]
fn assignee_env_cleared_when_absent() {
let mut command = Command::new("true");
apply_assignee_env(&mut command, None);
// `env_remove` records an explicit removal (key -> None) so the child
// cannot inherit a stale TSQ_TUI_ASSIGNEE from the parent environment.
let key = std::ffi::OsStr::new("TSQ_TUI_ASSIGNEE");
let found = command.get_envs().find(|(name, _)| *name == key);
assert_eq!(found, Some((key, None)));
}
🤖 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 `@src/cli/opentui.rs` around lines 267 - 307, The env-mutating tests in
opentui.rs are racing under parallel cargo test because they mutate shared
process state with set_var/remove_var. Serialize
interactive_gate_respects_disable_env and the existing env-based
binary-resolution test by guarding TSQ_OPENTUI_DISABLE/TSQ_OPENTUI_BIN mutations
with a shared Mutex or a test-serialization helper, and keep the environment
scoped so the original state is restored after each test. Use the test names and
the helper functions interactive_launch_ready and
resolves_explicit_bundled_tui_binary to locate the affected assertions.

Source: Coding guidelines

#[test]
fn resolves_explicit_bundled_tui_binary() {
let temp = tempfile::tempdir().expect("tempdir");
Expand Down
4 changes: 3 additions & 1 deletion tui-opentui/src/index.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import { createCliRenderer } from "@opentui/core";
import { createRoot } from "@opentui/react";
import { App } from "./tui-app";
import { WatchApp } from "./watch-app";

const mode = process.env.TSQ_TUI_MODE?.trim();
const renderer = await createCliRenderer();
createRoot(renderer).render(<App />);
createRoot(renderer).render(mode === "watch" ? <WatchApp /> : <App />);
Loading