feat(watch): dedicated OpenTUI live view for tsq watch#4
Merged
Conversation
Interactive `tsq watch` now launches a focused OpenTUI view (single scrollable task list mirroring the classic watch layout: header, summary, list, footer) instead of the flicker-prone full-screen ANSI-clear loop that broke scrolling on lists taller than the terminal. - Add WatchApp (watch-app.tsx) + testable helpers (watch-helpers.ts): reuses fetchTasks, sortTasks, computeSummary, buildTreeLines, THEME, visibleRange for windowed scrolling. Supports flat/tree, pause, j/k + g/G navigation, r refresh. - index.tsx branches on TSQ_TUI_MODE so one bundled binary serves both `tsq tui` (tabbed App) and `tsq watch` (WatchApp). - opentui.rs: add should_launch_opentui_watch / launch_opentui_watch, share launch/resolution logic; `tsq tui` clears inherited TSQ_TUI_MODE so it always renders the tabbed app. - meta.rs: execute_watch launches the OpenTUI view, falls back to the built-in Rust loop when unavailable / --json / --once / non-TTY. The `watch --once --json` contract is unchanged (it is the data source for both views), so the fallback loop and existing tests are untouched.
Address CodeRabbit review: - rowBudget under-counted vertical chrome (header/list/footer borders, margins, root padding, optional warning line), so the selected row could clip off the bottom near the end of a long list. Reserve all 16 non-list rows (17 with a warning) so visibleRange never overshoots. - Expand buildWatchRows tests: full status-priority ordering (flat), grandchild depth and unknown-parent-as-root (tree). Skipped: WatchApp interaction tests (timers/fetch/keyboard mocking) — no React component-test harness exists in this package; disproportionate for this change.
Summary by CodeRabbit
WalkthroughThis PR adds an optional OpenTUI watch-mode renderer. The Rust CLI can now launch a watch-specific TUI before falling back to the built-in renderer, and the TUI entrypoint switches between the normal app and the new watch app via ChangesOpenTUI Watch Mode
Estimated code review effort: 3 (Moderate) | ~30 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 3
🤖 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 `@src/cli/opentui.rs`:
- Around line 19-35: Add unit coverage for the new watch-launch gating logic in
should_launch_opentui_watch and interactive_launch_ready. Create tests in the
module’s existing test module that directly exercise the TSQ_OPENTUI_DISABLE
short-circuit and the json/once bypass paths, since these branches are now part
of the decision flow. Use the shared helper interactive_launch_ready to keep the
tests focused, and verify should_launch_opentui_watch respects the same gating
behavior when launch_target_available is involved.
- Around line 65-67: Both launch_opentui and launch_opentui_watch allow
TSQ_TUI_ASSIGNEE to inherit from the parent environment when options.assignee is
None. Update the Command setup in these functions to explicitly remove
TSQ_TUI_ASSIGNEE by default, and only set it when options.assignee is present,
matching the existing TSQ_TUI_MODE handling in launch_opentui. Use the
launch_opentui and launch_opentui_watch blocks that call Command::env and
env_remove to apply the fix in both places.
In `@tui-opentui/src/watch-app.tsx`:
- Around line 90-98: The row-budget math is currently duplicated as hard-coded
chrome numbers in WatchApp and the similar tableRowBudget calculation in App,
which risks future off-by-N clipping regressions. Move this arithmetic into a
small shared helper alongside visibleRange (for example in a shared helpers
module) that takes named chrome dimensions/margins and returns the budget, then
update WatchApp and the App/tableRowBudget path to call that helper so the
computation is centralized and testable.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: e4f52479-0b3b-45c6-98af-bc38a3481a70
📒 Files selected for processing (6)
src/cli/commands/meta.rssrc/cli/opentui.rstui-opentui/src/index.tsxtui-opentui/src/watch-app.tsxtui-opentui/src/watch-helpers.tstui-opentui/tests/watch-helpers.test.ts
… helper Address CodeRabbit PR #4 review: - Major: TSQ_TUI_ASSIGNEE could leak from the parent environment. Both launch_opentui and launch_opentui_watch now env_remove it when no assignee is requested (via shared apply_assignee_env), matching the existing TSQ_TUI_MODE handling, so a stale value can't silently filter a plain `tsq watch` / `tsq tui` run. - Add unit tests for the launch gating: json/once bypass, the TSQ_OPENTUI_DISABLE short-circuit, and apply_assignee_env set/clear. - Extract watchListRowBudget helper (watch-helpers.ts) for the list chrome math and unit-test it, so the off-by-N clipping class stays covered. Scoped to the watch view; the tabbed App layout is untouched.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tui-opentui/src/watch-app.tsx (1)
73-95: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSelection tracks list position, not task identity.
selectedIndexis a plain array index androwsis fully rebuilt/re-sorted on every poll (buildWatchRowsre-sorts by status/priority). The clamp effect (lines 87-89) only bounds-checks against the newrows.length— it doesn't verify the task at that index is still the one the user had highlighted. If a task's status changes (moving it in sort order) or the list otherwise reorders between polls, the highlighted row silently jumps to a different, unrelated task without any visual cue, which is confusing in a live-monitoring view where the whole point is to track specific tasks over time.Consider tracking the selected task's
idinstead of its index, and resolving the display index fromrowseach render (falling back to clamped index if the task disappears).♻️ Sketch of identity-based selection
- const [selectedIndex, setSelectedIndex] = useState(0); + const [selectedId, setSelectedId] = useState<string | undefined>(); ... - // Keep the selection valid as the live list grows or shrinks between polls. - useEffect(() => { - setSelectedIndex((current) => clampIndex(current, rows.length)); - }, [rows.length]); + const selectedIndex = useMemo(() => { + const idx = rows.findIndex((row) => row.task.id === selectedId); + return idx === -1 ? clampIndex(0, rows.length) : idx; + }, [rows, selectedId]);Update the keyboard handlers to compute the next
idviarows[nextIndex]?.task.idinstead of setting a raw index.🤖 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 `@tui-opentui/src/watch-app.tsx` around lines 73 - 95, Selection in watch-app.tsx is currently tied to a mutable array index, so when buildWatchRows re-sorts tasks on each poll the highlighted row can jump to a different task. Update the selection state and related keyboard/navigation logic in watch-app.tsx to track the selected task’s id instead of a raw index, then derive the display index from rows each render in the WatchApp flow; keep a clamped index fallback only when the selected task no longer exists. Use the existing selectedIndex, rows, buildWatchRows, and the visibleRange/render path to locate the affected logic.
🤖 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 `@src/cli/opentui.rs`:
- Around line 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.
---
Outside diff comments:
In `@tui-opentui/src/watch-app.tsx`:
- Around line 73-95: Selection in watch-app.tsx is currently tied to a mutable
array index, so when buildWatchRows re-sorts tasks on each poll the highlighted
row can jump to a different task. Update the selection state and related
keyboard/navigation logic in watch-app.tsx to track the selected task’s id
instead of a raw index, then derive the display index from rows each render in
the WatchApp flow; keep a clamped index fallback only when the selected task no
longer exists. Use the existing selectedIndex, rows, buildWatchRows, and the
visibleRange/render path to locate the affected logic.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 38db0b32-fd86-4ec7-933c-b7cc76259637
📒 Files selected for processing (4)
src/cli/opentui.rstui-opentui/src/watch-app.tsxtui-opentui/src/watch-helpers.tstui-opentui/tests/watch-helpers.test.ts
Comment on lines
+267
to
+307
| #[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))); | ||
| } | ||
|
|
There was a problem hiding this comment.
🩺 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
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.
Summary
Interactive
tsq watchnow launches a dedicated OpenTUI live view instead of the flicker-prone, full-screen ANSI-clear loop.The old loop redrew the whole list every tick with
\x1b[2J\x1b[H, never queried terminal height, and never windowed to a viewport. When the list was taller than the terminal, printing forced the terminal to scroll and the clear+home couldn't reclaim it — the top of the list became unreachable and frames flickered/accumulated. The new view is a single scrollable list that mirrors the classic watch layout (header · summary · list · footer) with proper viewport windowing.What changed
New dedicated watch UI (TypeScript / OpenTUI)
tui-opentui/src/watch-app.tsx—WatchApp: single live task list, flat/tree modes, pause,j/k+g/Gnavigation,rrefresh.tui-opentui/src/watch-helpers.ts— pure, testable helpers (buildWatchRows,filterLabel,metaBadge).tui-opentui/tests/watch-helpers.test.ts— unit tests.It reuses the existing plumbing —
fetchTasks,sortTasks,computeSummary,buildTreeLines,THEME,visibleRange,clampIndex— so the "watch" identity is just a simpler layout, not new data flow.Single binary, two modes
tui-opentui/src/index.tsxbranches onTSQ_TUI_MODE→<WatchApp/>for watch,<App/>for tui. No new bundled binary or CI artifact.Rust launcher / wiring
src/cli/opentui.rs— addedshould_launch_opentui_watch/launch_opentui_watch, sharing the entry/binary resolution with the tui path.tsq tuinowenv_removesTSQ_TUI_MODEso a leaked env var can't flip it into watch mode.src/cli/commands/meta.rs—execute_watchlaunches the OpenTUI view, falling back to the built-in Rust loop when OpenTUI is unavailable, or for--json/--once/ non-TTY.Contract preserved
tsq watch --once --jsonis the data source for both views, so it is unchanged — and the launcher guards (--json/--once/ non-TTY) mean the OpenTUI app's ownwatch --once --jsonsubprocess always takes the plain Rust path (no launch recursion). The Rust fallback loop and existing parity tests are untouched.Known caveat
TSQ_TUI_STATUSnarrows the list — watch's defaultopen,in_progressshows only those. Same client-side filtering model as the tui app.Verification
cargo fmt --check,cargo clippy --all-targets --all-features -- -D warnings— cleancargo test— 275 passbun run typecheck— clean;bun test— 43 pass (incl. new watch-helpers + real-binary contract test)Glands on last task,ppauses,tsq tuistill renders the tabbed app even withTSQ_TUI_MODE=watchleaked,--jsoncontract intact,TSQ_OPENTUI_DISABLE=1falls back to the Rust loop.CodeRabbit review
Ran
coderabbit review --agent(committed vsmain): 1 major + 3 trivial.rowBudgetunder-counted vertical chrome → selected row could clip at the bottom; now reserves all non-list rows incl. the optional warning line.buildWatchRowstests — full status ordering, grandchild depth, unknown-parent-as-root.