Skip to content

Commit

Permalink
fix(tui): start pty with matching TUI size (#9101)
Browse files Browse the repository at this point in the history
### Description

Fixes #9060

[From the
issue](#9060 (comment))

> I think I see the issue, it appears that rspack adds enough trailing
spaces in the progress bar output the fill the current line:
> 
> ```
> ● ESC[1mESC[0m
ESC[32mESC[37mESC[2m━━━━━━━━━━━━━━━━━━━━━━━━━ESC[0mESC[0m (0%)
ESC[2msetup compilation ESC[0m^MESC[2K
> ```
> 
> The `ESC[2K` operation at the erases the current line, but if the
trailing spaces have caused the line to wrap, it won't erase the
intended line instead just the line overflow. This probably indicates we
have a mismatch in the PTY dimensions and the virtual terminal
dimensions so `rspack` is outputting too many spaces for the terminal.
> 
> I'll look at making sure the underlying PTY has the exact same
dimensions as the virtual terminal.


There is future work to make sure that we update underlying PTY
instances if the pane changes sizes, but that requires a pretty heavy
refactor to our process management code to communicate with the PTY
"controller" as we only hold onto the "receiver".

### Testing Instructions

Before:
<img width="899" alt="Screenshot 2024-09-03 at 9 34 21 AM"
src="https://github.com/user-attachments/assets/b773457e-33ef-4075-aae4-9b0b9932a47a">


After:
<img width="483" alt="Screenshot 2024-09-03 at 9 30 20 AM"
src="https://github.com/user-attachments/assets/34997ea3-1032-4cf5-96db-b3a30e1c752e">
  • Loading branch information
chris-olszewski committed Sep 5, 2024
1 parent 8461559 commit 29fe5ef
Show file tree
Hide file tree
Showing 6 changed files with 110 additions and 35 deletions.
71 changes: 39 additions & 32 deletions crates/turborepo-lib/src/process/child.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ use tokio::{
};
use tracing::{debug, trace};

use super::Command;
use super::{Command, PtySize};

#[derive(Debug)]
pub enum ChildState {
Expand Down Expand Up @@ -135,22 +135,17 @@ impl ChildHandle {
}

#[tracing::instrument(skip(command))]
pub fn spawn_pty(command: Command) -> io::Result<SpawnResult> {
use portable_pty::PtySize;

pub fn spawn_pty(command: Command, size: PtySize) -> io::Result<SpawnResult> {
let keep_stdin_open = command.will_open_stdin();

let command = portable_pty::CommandBuilder::from(command);
let pty_system = native_pty_system();
let size =
console::Term::stdout()
.size_checked()
.map_or_else(PtySize::default, |(rows, cols)| PtySize {
rows,
cols,
pixel_width: 0,
pixel_height: 0,
});
let size = portable_pty::PtySize {
rows: size.rows,
cols: size.cols,
pixel_width: 0,
pixel_height: 0,
};
let pair = pty_system
.openpty(size)
.map_err(|err| match err.downcast() {
Expand Down Expand Up @@ -438,15 +433,15 @@ impl Child {
pub fn spawn(
command: Command,
shutdown_style: ShutdownStyle,
use_pty: bool,
pty_size: Option<PtySize>,
) -> io::Result<Self> {
let label = command.label();
let SpawnResult {
handle: mut child,
io: ChildIO { stdin, output },
controller,
} = if use_pty {
ChildHandle::spawn_pty(command)
} = if let Some(size) = pty_size {
ChildHandle::spawn_pty(command, size)
} else {
ChildHandle::spawn_normal(command)
}?;
Expand Down Expand Up @@ -832,7 +827,10 @@ mod test {
use turbopath::AbsoluteSystemPathBuf;

use super::{Child, ChildInput, ChildOutput, ChildState, Command};
use crate::process::child::{ChildExit, ShutdownStyle};
use crate::process::{
child::{ChildExit, ShutdownStyle},
PtySize,
};

const STARTUP_DELAY: Duration = Duration::from_millis(500);
// We skip testing PTY usage on Windows
Expand All @@ -855,7 +853,8 @@ mod test {
let script = find_script_dir().join_component("hello_world.js");
let mut cmd = Command::new("node");
cmd.args([script.as_std_path()]);
let mut child = Child::spawn(cmd, ShutdownStyle::Kill, use_pty).unwrap();
let mut child =
Child::spawn(cmd, ShutdownStyle::Kill, use_pty.then(PtySize::default)).unwrap();

assert_matches!(child.pid(), Some(_));
child.stop().await;
Expand All @@ -872,7 +871,8 @@ mod test {
let script = find_script_dir().join_component("hello_world.js");
let mut cmd = Command::new("node");
cmd.args([script.as_std_path()]);
let mut child = Child::spawn(cmd, ShutdownStyle::Kill, use_pty).unwrap();
let mut child =
Child::spawn(cmd, ShutdownStyle::Kill, use_pty.then(PtySize::default)).unwrap();

let exit1 = child.wait().await;
let exit2 = child.wait().await;
Expand All @@ -892,7 +892,8 @@ mod test {
cmd
};

let mut child = Child::spawn(cmd, ShutdownStyle::Kill, use_pty).unwrap();
let mut child =
Child::spawn(cmd, ShutdownStyle::Kill, use_pty.then(PtySize::default)).unwrap();

{
let state = child.state.read().await;
Expand All @@ -917,7 +918,8 @@ mod test {
let mut cmd = Command::new("node");
cmd.args([script.as_std_path()]);
cmd.open_stdin();
let mut child = Child::spawn(cmd, ShutdownStyle::Kill, use_pty).unwrap();
let mut child =
Child::spawn(cmd, ShutdownStyle::Kill, use_pty.then(PtySize::default)).unwrap();

tokio::time::sleep(STARTUP_DELAY).await;

Expand Down Expand Up @@ -959,7 +961,8 @@ mod test {
let mut cmd = Command::new("node");
cmd.args([script.as_std_path()]);
cmd.open_stdin();
let mut child = Child::spawn(cmd, ShutdownStyle::Kill, use_pty).unwrap();
let mut child =
Child::spawn(cmd, ShutdownStyle::Kill, use_pty.then(PtySize::default)).unwrap();

tokio::time::sleep(STARTUP_DELAY).await;

Expand Down Expand Up @@ -1006,7 +1009,7 @@ mod test {
let mut child = Child::spawn(
cmd,
ShutdownStyle::Graceful(Duration::from_millis(500)),
use_pty,
use_pty.then(PtySize::default),
)
.unwrap();

Expand Down Expand Up @@ -1043,7 +1046,7 @@ mod test {
let mut child = Child::spawn(
cmd,
ShutdownStyle::Graceful(Duration::from_millis(500)),
use_pty,
use_pty.then(PtySize::default),
)
.unwrap();

Expand Down Expand Up @@ -1073,7 +1076,7 @@ mod test {
let mut child = Child::spawn(
cmd,
ShutdownStyle::Graceful(Duration::from_millis(500)),
use_pty,
use_pty.then(PtySize::default),
)
.unwrap();

Expand Down Expand Up @@ -1122,7 +1125,8 @@ mod test {
let mut cmd = Command::new("node");
cmd.args([script.as_std_path()]);
cmd.open_stdin();
let mut child = Child::spawn(cmd, ShutdownStyle::Kill, use_pty).unwrap();
let mut child =
Child::spawn(cmd, ShutdownStyle::Kill, use_pty.then(PtySize::default)).unwrap();

let mut out = Vec::new();

Expand All @@ -1144,7 +1148,8 @@ mod test {
let mut cmd = Command::new("node");
cmd.args([script.as_std_path()]);
cmd.open_stdin();
let mut child = Child::spawn(cmd, ShutdownStyle::Kill, use_pty).unwrap();
let mut child =
Child::spawn(cmd, ShutdownStyle::Kill, use_pty.then(PtySize::default)).unwrap();

let mut buffer = Vec::new();

Expand All @@ -1168,7 +1173,8 @@ mod test {
let mut cmd = Command::new("node");
cmd.args([script.as_std_path()]);
cmd.open_stdin();
let mut child = Child::spawn(cmd, ShutdownStyle::Kill, use_pty).unwrap();
let mut child =
Child::spawn(cmd, ShutdownStyle::Kill, use_pty.then(PtySize::default)).unwrap();

let mut out = Vec::new();

Expand All @@ -1189,7 +1195,8 @@ mod test {
let mut cmd = Command::new("node");
cmd.args([script.as_std_path()]);
cmd.open_stdin();
let mut child = Child::spawn(cmd, ShutdownStyle::Kill, use_pty).unwrap();
let mut child =
Child::spawn(cmd, ShutdownStyle::Kill, use_pty.then(PtySize::default)).unwrap();

let mut out = Vec::new();

Expand Down Expand Up @@ -1217,7 +1224,7 @@ mod test {
let mut child = Child::spawn(
cmd,
ShutdownStyle::Graceful(Duration::from_millis(100)),
use_pty,
use_pty.then(PtySize::default),
)
.unwrap();

Expand All @@ -1233,7 +1240,7 @@ mod test {
async fn test_orphan_process() {
let mut cmd = Command::new("sh");
cmd.args(["-c", "echo hello; sleep 120; echo done"]);
let mut child = Child::spawn(cmd, ShutdownStyle::Kill, false).unwrap();
let mut child = Child::spawn(cmd, ShutdownStyle::Kill, None).unwrap();

tokio::time::sleep(STARTUP_DELAY).await;

Expand Down Expand Up @@ -1267,7 +1274,7 @@ mod test {
let script = find_script_dir().join_component("hello_world.js");
let mut cmd = Command::new("node");
cmd.args([script.as_std_path()]);
let child = Child::spawn(cmd, ShutdownStyle::Kill, use_pty).unwrap();
let child = Child::spawn(cmd, ShutdownStyle::Kill, use_pty.then(PtySize::default)).unwrap();

let mut stops = FuturesUnordered::new();
for _ in 1..10 {
Expand Down
38 changes: 37 additions & 1 deletion crates/turborepo-lib/src/process/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,13 @@ pub struct ProcessManager {
struct ProcessManagerInner {
is_closing: bool,
children: Vec<child::Child>,
size: Option<PtySize>,
}

#[derive(Debug, Clone, Copy)]
pub struct PtySize {
rows: u16,
cols: u16,
}

impl ProcessManager {
Expand All @@ -48,6 +55,7 @@ impl ProcessManager {
state: Arc::new(Mutex::new(ProcessManagerInner {
is_closing: false,
children: Vec::new(),
size: None,
})),
use_pty,
}
Expand Down Expand Up @@ -99,10 +107,11 @@ impl ProcessManager {
debug!("process manager closing");
return None;
}
let pty_size = self.use_pty.then(|| lock.pty_size()).flatten();
let child = child::Child::spawn(
command,
child::ShutdownStyle::Graceful(stop_timeout),
self.use_pty,
pty_size,
);
if let Ok(child) = &child {
lock.children.push(child.clone());
Expand Down Expand Up @@ -161,6 +170,33 @@ impl ProcessManager {
lock.children = vec![];
}
}

pub fn set_pty_size(&self, rows: u16, cols: u16) {
self.state.lock().expect("not poisoned").size = Some(PtySize { rows, cols });
}
}

impl ProcessManagerInner {
fn pty_size(&mut self) -> Option<PtySize> {
if self.size.is_none() {
self.size = PtySize::from_tty();
}
self.size
}
}

impl PtySize {
fn from_tty() -> Option<Self> {
console::Term::stdout()
.size_checked()
.map(|(rows, cols)| Self { rows, cols })
}
}

impl Default for PtySize {
fn default() -> Self {
Self { rows: 24, cols: 80 }
}
}

#[cfg(test)]
Expand Down
7 changes: 7 additions & 0 deletions crates/turborepo-lib/src/task_graph/visitor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,13 @@ impl<'a> Visitor<'a> {

let sink = Self::sink(run_opts);
let color_cache = ColorSelector::default();
// Set up correct size for underlying pty
if let Some(pane_size) = experimental_ui_sender
.as_ref()
.and_then(|sender| sender.pane_size())
{
manager.set_pty_size(pane_size.rows, pane_size.cols);
}

Self {
color_cache,
Expand Down
11 changes: 10 additions & 1 deletion crates/turborepo-ui/src/tui/app.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ const FRAMERATE: Duration = Duration::from_millis(3);
const RESIZE_DEBOUNCE_DELAY: Duration = Duration::from_millis(10);

use super::{
event::{CacheResult, Direction, OutputLogs, TaskResult},
event::{CacheResult, Direction, OutputLogs, PaneSize, TaskResult},
input,
search::SearchResults,
AppReceiver, Debouncer, Error, Event, InputOptions, SizeInfo, TaskTable, TerminalPane,
Expand Down Expand Up @@ -771,6 +771,15 @@ fn update(
Event::SearchBackspace => {
app.search_remove_char()?;
}
Event::PaneSizeQuery(callback) => {
// If caller has already hung up do nothing
callback
.send(PaneSize {
rows: app.size.pane_rows(),
cols: app.size.pane_cols(),
})
.ok();
}
}
Ok(None)
}
Expand Down
7 changes: 7 additions & 0 deletions crates/turborepo-ui/src/tui/event.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ pub enum Event {
status: String,
result: CacheResult,
},
PaneSizeQuery(std::sync::mpsc::SyncSender<PaneSize>),
Stop(std::sync::mpsc::SyncSender<()>),
// Stop initiated by the TUI itself
InternalStop,
Expand Down Expand Up @@ -88,6 +89,12 @@ pub enum OutputLogs {
ErrorsOnly,
}

#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub struct PaneSize {
pub rows: u16,
pub cols: u16,
}

#[cfg(test)]
mod test {
use super::*;
Expand Down
11 changes: 10 additions & 1 deletion crates/turborepo-ui/src/tui/handle.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use std::{
};

use super::{
event::{CacheResult, OutputLogs},
event::{CacheResult, OutputLogs, PaneSize},
Event, TaskResult,
};

Expand Down Expand Up @@ -72,6 +72,15 @@ impl AppSender {
pub fn restart_tasks(&self, tasks: Vec<String>) -> Result<(), mpsc::SendError<Event>> {
self.primary.send(Event::RestartTasks { tasks })
}

/// Fetches the size of the terminal pane
pub fn pane_size(&self) -> Option<PaneSize> {
let (callback_tx, callback_rx) = mpsc::sync_channel(1);
// Send query, if no receiver to handle the request return None
self.primary.send(Event::PaneSizeQuery(callback_tx)).ok()?;
// Wait for callback to be sent
callback_rx.recv().ok()
}
}

impl AppReceiver {
Expand Down

0 comments on commit 29fe5ef

Please sign in to comment.