diff --git a/EMBEDDING.md b/EMBEDDING.md index ae149336..8a47c181 100644 --- a/EMBEDDING.md +++ b/EMBEDDING.md @@ -153,6 +153,17 @@ function TerminalScreen() { `BlitSurfaceView` renders a single Wayland surface from a terminal's compositor. The server encodes each surface as H.264 or AV1; the component decodes via WebCodecs and draws to a canvas. +By default the view owns its surface's size: it resizes the surface to fill its +container, and is fully interactive. Pass `resizable={false}` for a passive +preview — a dock card, a switcher thumbnail — that shares another view's stream. +Such a view is served a fixed downscale capped at a thumbnail cadence and takes +no input at all, so it is the wrong choice for anything the user clicks in. + +`zoom` scales the surface independently of the pane's pixel size: `zoomMode` +`"relative"` (the default) multiplies the display's DPI by `zoom`, while +`"exact"` uses `zoom` as the absolute surface scale. Only resizable views drive +the scale. + ```tsx import { BlitSurfaceView } from "@blit-sh/react"; diff --git a/crates/cli/src/agent.rs b/crates/cli/src/agent.rs index f0297ef0..7830c143 100644 --- a/crates/cli/src/agent.rs +++ b/crates/cli/src/agent.rs @@ -4,20 +4,20 @@ use blit_remote::{AXIS_SOURCE_FINGER, AXIS_SOURCE_WHEEL, PointerAxisEvent}; use blit_remote::{ C2S_CLIENT_FEATURES, C2S_CLIENT_LIST, C2S_SURFACE_ACK, C2S_SURFACE_CAPTURE, C2S_SURFACE_LIST, C2S_SURFACE_POINTER, CAPTURE_FORMAT_AVIF, CAPTURE_FORMAT_PNG, CODEC_SUPPORT_AV1, - CODEC_SUPPORT_AV1_444, CODEC_SUPPORT_H264, CODEC_SUPPORT_H264_444, CREATE2_WANT_STATUS, + CODEC_SUPPORT_AV1_444, CODEC_SUPPORT_H264, CODEC_SUPPORT_H264_444, Create2Request, EXIT_REASON_NORMAL, EXIT_STATUS_UNKNOWN, FEATURE_CLIENT_CONTROL, FEATURE_CLIENT_ORIGIN, - FEATURE_CREATE_STATUS, FEATURE_PTY_DEADLINE, KICK_REASON_MAX, S2C_CLIPBOARD_CONTENT, - S2C_CLIPBOARD_LIST, S2C_EXITED, S2C_HELLO, S2C_KICKED, S2C_LIST, S2C_PING, S2C_QUIT, S2C_READY, - S2C_SURFACE_CAPTURE, S2C_SURFACE_FRAME, S2C_SURFACE_LIST, S2C_TERM_CWD, S2C_TEXT, S2C_TITLE, - S2C_UPDATE, STATUS_OK, SURFACE_FRAME_CODEC_AV1, SURFACE_FRAME_CODEC_MASK, - SURFACE_FRAME_FLAG_KEYFRAME, ServerMsg, TerminalState, exit_reason_text, msg_ack, - msg_c2s_clipboard_get, msg_c2s_clipboard_list, msg_c2s_clipboard_set, msg_c2s_primary_set, - msg_client_list, msg_client_list_with_origin, msg_close, msg_create2_full, msg_deadline, - msg_display_rate, msg_input, msg_kick, msg_kill, msg_mouse, msg_quit, msg_read, msg_resize, - msg_restart, msg_subscribe, msg_surface_close, msg_surface_focus, msg_surface_input, - msg_surface_pointer_axis2, msg_surface_resize, msg_surface_subscribe, - msg_surface_subscribe_ext, msg_surface_subscribe_scaled, msg_surface_text, msg_term_cwd, - parse_server_msg, parse_term_cwd_reply, status_text, + FEATURE_CREATE_EXEC, FEATURE_CREATE_STATUS, FEATURE_PTY_DEADLINE, KICK_REASON_MAX, + S2C_CLIPBOARD_CONTENT, S2C_CLIPBOARD_LIST, S2C_EXITED, S2C_HELLO, S2C_KICKED, S2C_LIST, + S2C_PING, S2C_QUIT, S2C_READY, S2C_SURFACE_CAPTURE, S2C_SURFACE_FRAME, S2C_SURFACE_LIST, + S2C_TERM_CWD, S2C_TEXT, S2C_TITLE, S2C_UPDATE, STATUS_OK, SURFACE_FRAME_CODEC_AV1, + SURFACE_FRAME_CODEC_MASK, SURFACE_FRAME_FLAG_KEYFRAME, ServerMsg, TerminalState, + exit_reason_text, msg_ack, msg_c2s_clipboard_get, msg_c2s_clipboard_list, + msg_c2s_clipboard_set, msg_c2s_primary_set, msg_client_list, msg_client_list_with_origin, + msg_close, msg_create2_request, msg_deadline, msg_display_rate, msg_input, msg_kick, msg_kill, + msg_mouse, msg_quit, msg_read, msg_resize, msg_restart, msg_subscribe, msg_surface_close, + msg_surface_focus, msg_surface_input, msg_surface_pointer_axis2, msg_surface_resize, + msg_surface_subscribe, msg_surface_subscribe_ext, msg_surface_subscribe_scaled, + msg_surface_text, msg_term_cwd, parse_server_msg, parse_term_cwd_reply, status_text, }; use tokio::io::{AsyncRead, AsyncReadExt, AsyncWrite, AsyncWriteExt}; @@ -475,40 +475,95 @@ pub async fn cmd_deadline(transport: Transport, id: u16, seconds: u64) -> Result Ok(()) } -pub async fn cmd_start( - transport: Transport, - tag: Option, - command: Vec, - rows: u16, - cols: u16, - deadline: Option, -) -> Result { +/// Split a `KEY=VALUE` pair the way `env(1)` does: on the first `=`. +pub fn parse_env_assignment(entry: &str) -> Result<(&str, &str), String> { + match entry.split_once('=') { + Some(("", _)) => Err(format!("--env needs a name before the '=': {entry:?}")), + Some(pair) => Ok(pair), + None => Err(format!("--env needs KEY=VALUE, got {entry:?}")), + } +} + +/// Characters that mean something to a shell and nothing to `execve`. +/// A lone word carrying one of these is almost always a shell line someone +/// expected to be interpreted, and silently exec'ing it fails with a bare +/// `No such file or directory` naming the whole line. +const SHELL_SYNTAX: &[char] = &['|', '&', ';', '<', '>', '(', ')', '$', '`', '*', '?', '\n']; + +pub struct StartRequest { + pub tag: Option, + pub command: Vec, + pub shell: bool, + pub cwd: Option, + pub env: Vec, + pub rows: u16, + pub cols: u16, + pub deadline: Option, +} + +pub async fn cmd_start(transport: Transport, req: StartRequest) -> Result { let mut conn = AgentConn::connect(transport).await?; - if deadline.is_some() && conn.features & FEATURE_PTY_DEADLINE == 0 { + if req.deadline.is_some() && conn.features & FEATURE_PTY_DEADLINE == 0 { return Err("server does not support deadlines".to_string()); } + let exec = conn.features & FEATURE_CREATE_EXEC != 0; + let env: Vec<(&str, &str)> = req + .env + .iter() + .map(|entry| parse_env_assignment(entry)) + .collect::>()?; + if !env.is_empty() && !exec { + return Err( + "server does not support setting a terminal's environment (needs a newer blit server)" + .to_string(), + ); + } + if req.shell && req.command.is_empty() { + return Err("--shell needs a command to run".to_string()); + } + if !req.shell + && let [only] = req.command.as_slice() + && only.contains(SHELL_SYNTAX) + { + return Err(format!( + "{only:?} looks like shell syntax, but commands are executed directly.\n\ + Pass --shell to run it through the server's shell." + )); + } + let nonce: u16 = 1; - let tag_str = tag.as_deref().unwrap_or(""); - let cmd_str = command.join("\0"); - // Only ask for a correlated outcome from a server that advertised it — - // an older one would drop the flag byte's meaning and still answer - // nothing on refusal (docs/protocol.md, "Common status registry"). - let features = if conn.features & FEATURE_CREATE_STATUS != 0 { - CREATE2_WANT_STATUS + // A shell command is one string; anything else is an argv. Against a + // server too old for HAS_ARGV, spell the argv the legacy way: NUL-joined + // under HAS_COMMAND, with a trailing NUL so even a one-word argv carries + // one and takes the server's argv branch rather than its shell branch. + let legacy_argv = (!req.shell && !req.command.is_empty() && !exec) + .then(|| format!("{}\0", req.command.join("\0"))); + let command = if req.shell { + Some(req.command.join(" ")) } else { - 0 + legacy_argv }; - let msg = msg_create2_full( + let argv: Option> = (!req.shell && !req.command.is_empty() && exec) + .then(|| req.command.iter().map(String::as_str).collect()); + + let msg = msg_create2_request(&Create2Request { nonce, - rows, - cols, - tag_str, - &cmd_str, - features, - None, - deadline.map(deadline_ms), - ); + rows: req.rows, + cols: req.cols, + // Only ask for a correlated outcome from a server that advertised it — + // an older one would drop the flag byte's meaning and still answer + // nothing on refusal (docs/protocol.md, "Common status registry"). + want_status: conn.features & FEATURE_CREATE_STATUS != 0, + tag: req.tag.as_deref().unwrap_or(""), + cwd: req.cwd.as_deref(), + deadline_ms: req.deadline.map(deadline_ms), + env, + argv, + command: command.as_deref(), + ..Default::default() + }) + .map_err(|err| format!("cannot start terminal: {}", err.detail))?; conn.send(&msg).await?; loop { @@ -2637,6 +2692,127 @@ mod tests { mock.await.unwrap(); } + fn start_request(command: &[&str]) -> StartRequest { + StartRequest { + tag: None, + command: command.iter().map(|c| (*c).to_string()).collect(), + shell: false, + cwd: None, + env: Vec::new(), + rows: 24, + cols: 80, + deadline: None, + } + } + + /// Drive `cmd_start` and hand back the `CREATE2` it put on the wire. + async fn started_message(features: u32, req: StartRequest) -> Result, String> { + let (client, server) = tokio::net::UnixStream::pair().unwrap(); + let mock = tokio::spawn(async move { + let mut mock = MockServer::new(server); + mock.send_initial_burst_with_features(features).await; + let data = mock.recv().await?; + let nonce = u16::from_le_bytes([data[1], data[2]]); + mock.send_created_n(nonce, 5, "").await; + Some(data) + }); + let result = cmd_start(Transport::Unix(client), req).await; + let sent = mock.await.unwrap(); + result.map(|_| sent.expect("server saw no create")) + } + + /// The headline change: a bare command word is exec'd, not handed to a + /// login shell. It used to depend on how many words you typed. + #[tokio::test] + async fn start_sends_an_argv_to_a_server_that_can_exec() { + for command in [&["htop"][..], &["ls", "-la"][..]] { + let sent = started_message(FEATURE_CREATE_EXEC, start_request(command)) + .await + .unwrap(); + let req = blit_remote::parse_create2(&sent).unwrap(); + assert_eq!(req.argv.as_deref(), Some(command)); + assert_eq!(req.command, None); + } + } + + /// Against a server too old for `HAS_ARGV`, the same request still has to + /// exec. The trailing NUL is what makes a one-word argv take the old + /// server's argv branch instead of its shell branch. + #[tokio::test] + async fn start_falls_back_to_the_legacy_argv_spelling() { + for command in [&["htop"][..], &["ls", "-la"][..]] { + let sent = started_message(0, start_request(command)).await.unwrap(); + let req = blit_remote::parse_create2(&sent).unwrap(); + assert_eq!(req.argv.as_deref(), Some(command)); + assert_eq!(req.command, None); + assert_eq!(sent[7] & blit_remote::CREATE2_HAS_ARGV, 0); + assert_ne!(sent[7] & blit_remote::CREATE2_HAS_COMMAND, 0); + } + } + + #[tokio::test] + async fn start_shell_flag_sends_one_command_string() { + let mut req = start_request(&["ls", "|", "wc", "-l"]); + req.shell = true; + let sent = started_message(FEATURE_CREATE_EXEC, req).await.unwrap(); + let parsed = blit_remote::parse_create2(&sent).unwrap(); + assert_eq!(parsed.command, Some("ls | wc -l")); + assert_eq!(parsed.argv, None); + } + + #[tokio::test] + async fn start_carries_cwd_and_environment() { + let mut req = start_request(&["env"]); + req.cwd = Some("/tmp".to_string()); + req.env = vec!["FOO=bar".to_string(), "EMPTY=".to_string()]; + let sent = started_message(FEATURE_CREATE_EXEC, req).await.unwrap(); + let parsed = blit_remote::parse_create2(&sent).unwrap(); + assert_eq!(parsed.cwd, Some("/tmp")); + assert_eq!(parsed.env, vec![("FOO", "bar"), ("EMPTY", "")]); + } + + /// An environment nobody will apply is worse than a refusal: the terminal + /// starts and the variables are simply not there. + #[tokio::test] + async fn start_refuses_an_environment_an_old_server_would_drop() { + let mut req = start_request(&["env"]); + req.env = vec!["FOO=bar".to_string()]; + let err = started_message(0, req).await.unwrap_err(); + assert!(err.contains("environment"), "{err}"); + } + + /// Exec-by-default turns a shell one-liner into a program name with spaces + /// in it, which fails as an unreadable ENOENT. Say what to do instead. + #[tokio::test] + async fn start_names_shell_syntax_rather_than_exec_ing_it() { + let err = started_message(FEATURE_CREATE_EXEC, start_request(&["ls | wc -l"])) + .await + .unwrap_err(); + assert!(err.contains("--shell"), "{err}"); + // A word that merely looks unusual is still a program name. + assert!( + started_message( + FEATURE_CREATE_EXEC, + start_request(&["/opt/my-app/bin/run-it_2.0"]) + ) + .await + .is_ok() + ); + } + + #[test] + fn env_assignments_split_on_the_first_equals() { + assert_eq!(parse_env_assignment("FOO=bar"), Ok(("FOO", "bar"))); + assert_eq!(parse_env_assignment("FOO="), Ok(("FOO", ""))); + // A value may contain '=' — only the key may not. + assert_eq!( + parse_env_assignment("URL=http://x/?a=b"), + Ok(("URL", "http://x/?a=b")) + ); + assert!(parse_env_assignment("FOO").is_err()); + assert!(parse_env_assignment("=bar").is_err()); + } + #[tokio::test] async fn test_show() { let (client, server) = tokio::net::UnixStream::pair().unwrap(); diff --git a/crates/cli/src/cli.rs b/crates/cli/src/cli.rs index 669a1fc7..bef450df 100644 --- a/crates/cli/src/cli.rs +++ b/crates/cli/src/cli.rs @@ -668,10 +668,40 @@ pub enum TerminalCommand { List, /// Start a new terminal and print its ID + /// + /// The command is executed directly, the way a process is started anywhere + /// else — no login shell, so no rc files and no shell syntax. Pass --shell + /// to run one string through $SHELL instead. With no command at all, the + /// terminal gets the default interactive shell. + /// + /// Options come before the command; everything after the first bare word + /// belongs to it. Use -- when the command's own flags would be ambiguous. + /// + /// Examples: + /// blit terminal start htop + /// blit terminal start -- cargo test --release + /// blit terminal start --cwd /src --env RUST_LOG=debug -- cargo run + /// blit terminal start --shell 'ls | wc -l' Start { /// Command to run (defaults to $SHELL or /bin/sh) + #[arg(trailing_var_arg = true, allow_hyphen_values = true)] command: Vec, + /// Run the command through the server's login shell ($SHELL -lic) + /// instead of executing it directly. Needed for pipes, redirections, + /// globs, and anything else that is shell syntax rather than a program. + #[arg(long, short = 'c')] + shell: bool, + + /// Working directory for the new terminal + #[arg(long, value_name = "DIR")] + cwd: Option, + + /// Set an environment variable, repeatable (--env KEY=VALUE). + /// Overrides whatever the server would otherwise pass down. + #[arg(long, value_name = "KEY=VALUE")] + env: Vec, + /// Terminal tag / label #[arg(long, short = 't')] tag: Option, diff --git a/crates/cli/src/learn.md b/crates/cli/src/learn.md index edd10d3d..ff77b7f8 100644 --- a/crates/cli/src/learn.md +++ b/crates/cli/src/learn.md @@ -11,6 +11,20 @@ ID=$(blit terminal start --cols 200) # start a shell Always use `--cols 200` or wider to avoid line wrapping. Tag terminals with `-t`. +The command is executed directly — no login shell, so no rc files and no shell +syntax. For a pipe, a redirection, a glob, or a `&&`, ask for the shell: + +```bash +blit terminal start --cols 200 --shell 'make 2>&1 | tail -40' +``` + +Pass a working directory and environment variables the same way you would to +any other program. `--env` is repeatable, and options go _before_ the command: + +```bash +blit terminal start --cols 200 --cwd /src/blit --env RUST_LOG=debug -- cargo run +``` + `start` returns immediately. Use `--wait --timeout N` to block until completion: ```bash diff --git a/crates/cli/src/main.rs b/crates/cli/src/main.rs index 8bf5a9d0..b6834cc2 100644 --- a/crates/cli/src/main.rs +++ b/crates/cli/src/main.rs @@ -135,6 +135,9 @@ async fn async_main() { TerminalCommand::List => agent::cmd_list(transport).await, TerminalCommand::Start { command, + shell, + cwd, + env, tag, rows, cols, @@ -142,8 +145,20 @@ async fn async_main() { timeout, deadline, } => { - let start_result = - agent::cmd_start(transport, tag, command, rows, cols, deadline).await; + let start_result = agent::cmd_start( + transport, + agent::StartRequest { + tag, + command, + shell, + cwd, + env, + rows, + cols, + deadline, + }, + ) + .await; if wait { let pty_id = match start_result { Ok(id) => id, diff --git a/crates/compositor/src/imp.rs b/crates/compositor/src/imp.rs index d08b7c55..ff61c592 100644 --- a/crates/compositor/src/imp.rs +++ b/crates/compositor/src/imp.rs @@ -593,8 +593,10 @@ pub enum CompositorEvent { SurfaceDestroyed { surface_id: u16, }, - /// A client asked to activate (raise/focus) a surface via - /// xdg_activation_v1; forwarded so the frontend can raise the pane. + /// A client asked to activate a surface via xdg_activation_v1; forwarded so + /// the frontend can point the viewer at it. Not a raise: the frontend + /// answers with a highlight, because clients repeat this request and each + /// repeat would otherwise land on top of whatever the viewer just chose. SurfaceActivated { surface_id: u16, }, @@ -11213,10 +11215,17 @@ impl Dispatch for Compositor { // Tokens are issued unvalidated, so there is nothing to // check here. Pane focus is managed externally by the // browser/CLI, but "always granted" must not mean "silently - // dropped": forward the request so the frontend can raise - // and focus the matching pane. An ignored activation is - // what strands an Electron app that asks to come back - // (Slack on a notification click) behind everything else. + // dropped": forward the request so the frontend can point the + // viewer at the surface. An ignored activation is what + // strands an Electron app that asks to come back (Slack on a + // notification click) behind everything else. + // + // Forwarding every repeat is safe *because* the frontend + // answers with a highlight and not the view — when it raised + // instead, a client asking several times a second could not be + // clicked away from. A compositor that wanted to grant this + // literally would first have to validate the token against a + // recent input serial belonging to the requesting client. if let Some(surf) = state.surfaces.get(&surface.id()) && surf.surface_id > 0 { diff --git a/crates/compositor/src/lib.rs b/crates/compositor/src/lib.rs index 9f179996..0d21acea 100644 --- a/crates/compositor/src/lib.rs +++ b/crates/compositor/src/lib.rs @@ -204,7 +204,9 @@ mod stub { /// The client asked for one of its toplevels to be activated /// (xdg_activation_v1) — e.g. an Electron app reacting to a /// notification click. Pane focus belongs to the frontend, so the - /// request is forwarded, not acted on here. + /// request is forwarded, not acted on here — and the frontend answers + /// it with a highlight rather than the view, since a client may repeat + /// the request indefinitely. SurfaceActivated { surface_id: u16, }, diff --git a/crates/desktop/src/lib.rs b/crates/desktop/src/lib.rs index ce89108d..10b24c1b 100644 --- a/crates/desktop/src/lib.rs +++ b/crates/desktop/src/lib.rs @@ -1531,13 +1531,23 @@ async fn monitor_menu( return; }; loop { - let live = tokio::select! { - signal = layouts.next() => signal.is_some(), - signal = properties.next() => signal.is_some(), + // A layout change can renumber or repurpose every id, so nothing the + // client is holding can be trusted afterwards. A property change + // cannot: it repaints named items and leaves the rest of the menu + // exactly as the user is reading it. Apps repaint constantly — Zoom + // re-syncs its language checkmark on every AboutToShow — and voiding + // the whole menu for that dropped the click the user was in the middle + // of making, with no way to tell them why. + let change = tokio::select! { + signal = layouts.next() => match signal { + Some(_) => MenuChange::Layout, + None => return, + }, + signal = properties.next() => match signal { + Some(signal) => repainted_menu_items(&signal), + None => return, + }, }; - if !live { - return; - } let mut watcher = state.lock().await; let Some(target) = watcher.items.get_mut(&key) else { return; @@ -1545,11 +1555,47 @@ async fn monitor_menu( if target.menu_path.as_ref() != Some(&path) { return; } - target.menu_revision = 0; - target.menu_items.clear(); + match change { + MenuChange::Layout => { + target.menu_revision = 0; + target.menu_items.clear(); + } + // The repainted items are the ones the client's copy now + // misdescribes, so only those stop being clickable; the menu keeps + // its revision and every other item stays live. + MenuChange::Repainted(ids) => target.menu_items.retain(|id, _| !ids.contains(id)), + } } } +enum MenuChange { + /// The menu was restructured: every id the client holds may now name a + /// different item, so none of them can be acted on. + Layout, + /// These items were repainted; the rest of the menu is still what the client + /// is showing. + Repainted(HashSet), +} + +/// Which items an `ItemsPropertiesUpdated` signal repaints. An unreadable body +/// has to be treated as if it changed everything. +fn repainted_menu_items(signal: &zbus::Message) -> MenuChange { + type PropertiesUpdated = ( + Vec<(i32, HashMap)>, + Vec<(i32, Vec)>, + ); + let Ok((updated, removed)) = signal.body().deserialize::() else { + return MenuChange::Layout; + }; + MenuChange::Repainted( + updated + .into_iter() + .map(|(id, _)| id) + .chain(removed.into_iter().map(|(id, _)| id)) + .collect(), + ) +} + fn menu_string<'a>(properties: &'a HashMap, name: &str) -> Option<&'a str> { properties .get(name) @@ -3839,4 +3885,127 @@ mod dbus_tests { // Keep the connection observably live until normalization completed. assert!(item_connection.unique_name().is_some()); } + + /// Real apps repaint their tray menu while it is on screen — Zoom re-syncs + /// its language checkmark on every `AboutToShow`, and does so again + /// whenever its own state changes. The click the user is in the middle of + /// making has to survive that: the menu they are looking at still names the + /// item they are pointing at. + #[tokio::test] + async fn a_property_update_does_not_swallow_the_click_on_the_open_menu() { + let (_bus, address) = TestBus::spawn(); + let mut bridge = Bridge::start(&address, Config::default(), Arc::new(|| {})) + .await + .unwrap(); + let clicked = Arc::new(AtomicI32::new(0)); + let item_connection = zbus::connection::Builder::address(address.as_str()) + .unwrap() + .name("org.example.BlitChurn") + .unwrap() + .serve_at("/StatusNotifierItem", MockItem) + .unwrap() + .serve_at( + "/Menu", + MockMenu { + clicked: clicked.clone(), + }, + ) + .unwrap() + .build() + .await + .unwrap(); + let watcher = Proxy::new( + &item_connection, + "org.kde.StatusNotifierWatcher", + WATCHER_PATH, + "org.kde.StatusNotifierWatcher", + ) + .await + .unwrap(); + watcher + .call::<_, _, ()>("RegisterStatusNotifierItem", &("org.example.BlitChurn",)) + .await + .unwrap(); + let Event::Tray(TrayRecord::Upsert(item)) = next_event(&mut bridge).await else { + panic!("expected tray upsert") + }; + + assert!(bridge.try_command(Command::Tray(TrayEvent { + tray_id: item.tray_id, + kind: TRAY_EVENT_OPEN_MENU, + menu_revision: 0, + value: 0, + flags: 0, + }))); + let Event::TrayMenu(menu) = next_event(&mut bridge).await else { + panic!("expected normalized tray menu") + }; + assert_eq!(menu.status, TRAY_MENU_OK); + + // The app repaints one item while the menu sits open in front of the + // user — Zoom's own signal touches its language checkmark, never the + // Exit the user is reaching for. + let repaint = async |id: i32| { + let updated: Vec<(i32, HashMap)> = vec![( + id, + HashMap::from([("toggle-state".to_string(), OwnedValue::from(0i32))]), + )]; + let removed: Vec<(i32, Vec)> = Vec::new(); + item_connection + .emit_signal( + None::<&str>, + "/Menu", + "com.canonical.dbusmenu", + "ItemsPropertiesUpdated", + &(updated, removed), + ) + .await + .unwrap(); + sleep(Duration::from_millis(200)).await; + }; + + repaint(4).await; + assert!(bridge.try_command(Command::Tray(TrayEvent { + tray_id: item.tray_id, + kind: TRAY_EVENT_MENU_ITEM, + menu_revision: menu.menu_revision, + value: 2, + flags: 0, + }))); + let Event::TrayMenu(after) = next_event(&mut bridge).await else { + panic!("expected a menu event after the click") + }; + assert_eq!( + after.status, TRAY_MENU_OK, + "a repaint elsewhere must not make the open menu stale" + ); + assert_eq!( + clicked.load(Ordering::Relaxed), + 2, + "the click must reach the app" + ); + + // The item that *was* repainted is the one the client now misdescribes, + // so a click on it is still refused and answered with a fresh menu. + repaint(4).await; + assert!(bridge.try_command(Command::Tray(TrayEvent { + tray_id: item.tray_id, + kind: TRAY_EVENT_MENU_ITEM, + menu_revision: after.menu_revision, + value: 4, + flags: 0, + }))); + let Event::TrayMenu(reread) = next_event(&mut bridge).await else { + panic!("expected a menu event after the refused click") + }; + assert_eq!(reread.status, TRAY_MENU_OK); + assert_ne!(reread.menu_revision, after.menu_revision); + assert_eq!( + clicked.load(Ordering::Relaxed), + 2, + "a click on the repainted item must not reach the app" + ); + + assert!(item_connection.unique_name().is_some()); + } } diff --git a/crates/guest/src/terminal.rs b/crates/guest/src/terminal.rs index 0f9e79dd..aec2a679 100644 --- a/crates/guest/src/terminal.rs +++ b/crates/guest/src/terminal.rs @@ -10,8 +10,9 @@ use alloc::{string::String, vec::Vec}; use core::fmt; use blit_remote::{ - CREATE2_WANT_STATUS, FEATURE_CREATE_STATUS, FEATURE_PTY_DEADLINE, S2C_UPDATE, ServerMsg, - TerminalState, msg_ack, msg_create2_full, msg_subscribe, msg_unsubscribe, parse_server_msg, + Create2Request, FEATURE_CREATE_EXEC, FEATURE_CREATE_STATUS, FEATURE_PTY_DEADLINE, S2C_UPDATE, + ServerMsg, TerminalState, msg_ack, msg_create2_request, msg_subscribe, msg_unsubscribe, + parse_server_msg, }; use crate::{Client, Error as ClientError}; @@ -22,12 +23,20 @@ pub struct CreateRequest<'a> { pub rows: u16, pub cols: u16, pub tag: &'a str, + /// Run this through the server's login shell. Leave empty when using + /// `argv`; setting both is [`Error::CreateFailed`] with `INVALID`. pub command: &'a str, + /// Exec this argv directly, no shell. Needs + /// [`FEATURE_CREATE_EXEC`](blit_remote::FEATURE_CREATE_EXEC). + pub argv: Option<&'a [&'a str]>, pub cwd: Option<&'a str>, + /// Environment overrides, applied on top of everything the server derives. + /// Needs [`FEATURE_CREATE_EXEC`](blit_remote::FEATURE_CREATE_EXEC). + pub env: &'a [(&'a str, &'a str)], pub deadline_ms: Option, } -impl CreateRequest<'_> { +impl<'a> CreateRequest<'a> { /// A shell using the server's default command, tag, cwd, and lifetime. pub const fn shell(rows: u16, cols: u16) -> Self { Self { @@ -35,10 +44,20 @@ impl CreateRequest<'_> { cols, tag: "", command: "", + argv: None, cwd: None, + env: &[], deadline_ms: None, } } + + /// Exec `argv` directly, the way a process is started outside a terminal. + pub const fn exec(rows: u16, cols: u16, argv: &'a [&'a str]) -> Self { + Self { + argv: Some(argv), + ..Self::shell(rows, cols) + } + } } /// State held for one subscribed PTY. @@ -214,18 +233,33 @@ impl TerminalSubscriptions { if request.deadline_ms.is_some() && features & FEATURE_PTY_DEADLINE == 0 { return Err(Error::FeatureMissing("FEATURE_PTY_DEADLINE")); } + // Neither exec field is probeable: an older server ignores the flag and + // reads the block as command bytes, or spawns a plain shell. Refuse + // here rather than let it run something else. + if (request.argv.is_some() || !request.env.is_empty()) + && features & FEATURE_CREATE_EXEC == 0 + { + return Err(Error::FeatureMissing("FEATURE_CREATE_EXEC")); + } let nonce = self.allocate_create_nonce(); - let packet = msg_create2_full( + let packet = msg_create2_request(&Create2Request { nonce, - request.rows, - request.cols, - request.tag, - request.command, - CREATE2_WANT_STATUS, - request.cwd, - request.deadline_ms, - ); + rows: request.rows, + cols: request.cols, + want_status: true, + tag: request.tag, + cwd: request.cwd, + deadline_ms: request.deadline_ms, + env: request.env.to_vec(), + argv: request.argv.map(<[&str]>::to_vec), + command: (!request.command.is_empty()).then_some(request.command), + ..Default::default() + }) + .map_err(|err| Error::CreateFailed { + status: err.status, + detail: String::from(err.detail), + })?; client.send(&packet)?; let reply = client .recv_matching(|packet| creation_reply(packet, nonce))? @@ -528,13 +562,15 @@ mod tests { } } - fn hello() -> Vec { + const ALL_FEATURES: u32 = bootstrap::FEATURE_EXTENSION + | FEATURE_CREATE_STATUS + | FEATURE_PTY_DEADLINE + | FEATURE_CREATE_EXEC; + + fn hello_with(features: u32) -> Vec { let mut packet = vec![bootstrap::S2C_HELLO]; packet.extend_from_slice(&1u16.to_le_bytes()); - packet.extend_from_slice( - &(bootstrap::FEATURE_EXTENSION | FEATURE_CREATE_STATUS | FEATURE_PTY_DEADLINE) - .to_le_bytes(), - ); + packet.extend_from_slice(&features.to_le_bytes()); packet } @@ -553,11 +589,21 @@ mod tests { } fn boot() -> (native_host::Guard, Rc>, Client) { + boot_features(ALL_FEATURES) + } + + /// Boot against a server that does not advertise `missing`. + fn boot_without(missing: u32) -> (native_host::Guard, Rc>, Client) { + boot_features(ALL_FEATURES & !missing) + } + + fn boot_features(features: u32) -> (native_host::Guard, Rc>, Client) { let state = Rc::new(RefCell::new(State::default())); - state - .borrow_mut() - .incoming - .extend([hello(), vec![bootstrap::S2C_READY], init()]); + state.borrow_mut().incoming.extend([ + hello_with(features), + vec![bootstrap::S2C_READY], + init(), + ]); let guard = native_host::install(MockHost(Rc::clone(&state))); let client = Client::bootstrap().expect("valid extension bootstrap"); (guard, state, client) @@ -690,12 +736,11 @@ mod tests { .push_back(vec![S2C_CREATED_N, 1, 0, 33, 0]); let mut terminals = TerminalSubscriptions::new(); let request = CreateRequest { - rows: 24, - cols: 80, tag: "worker", command: "cargo test", cwd: Some("/work"), deadline_ms: Some(5_000), + ..CreateRequest::shell(24, 80) }; assert_eq!( @@ -706,8 +751,54 @@ mod tests { ); let sent = &state.borrow().sent; assert_eq!(sent[0][0], C2S_CREATE2); - assert_ne!(sent[0][7] & CREATE2_WANT_STATUS, 0); + assert_ne!(sent[0][7] & blit_remote::CREATE2_WANT_STATUS, 0); assert_eq!(sent[1], vec![C2S_SUBSCRIBE, 33, 0]); assert_eq!(ack_count(&state.borrow()), 0); } + + #[test] + fn an_exec_request_carries_argv_and_environment() { + let (_guard, state, mut client) = boot(); + state + .borrow_mut() + .incoming + .push_back(vec![S2C_CREATED_N, 1, 0, 7, 0]); + let mut terminals = TerminalSubscriptions::new(); + let request = CreateRequest { + cwd: Some("/work"), + env: &[("RUST_LOG", "debug")], + ..CreateRequest::exec(24, 80, &["cargo", "test", "--release"]) + }; + + assert_eq!(terminals.create(&mut client, request).unwrap(), 7); + let sent = &state.borrow().sent; + let parsed = blit_remote::parse_create2(&sent[0]).unwrap(); + assert_eq!( + parsed.argv.as_deref(), + Some(&["cargo", "test", "--release"][..]) + ); + assert_eq!(parsed.env, vec![("RUST_LOG", "debug")]); + assert_eq!(parsed.cwd, Some("/work")); + assert_eq!(parsed.command, None); + } + + /// An older server ignores the bit and spawns a plain shell, so asking for + /// an exec it cannot do has to fail rather than silently become something + /// else. + #[test] + fn an_exec_request_needs_the_feature_bit() { + let (_guard, _state, mut client) = boot_without(FEATURE_CREATE_EXEC); + let mut terminals = TerminalSubscriptions::new(); + let mut missing = |request| { + matches!( + terminals.create(&mut client, request), + Err(Error::FeatureMissing("FEATURE_CREATE_EXEC")) + ) + }; + assert!(missing(CreateRequest::exec(24, 80, &["htop"]))); + assert!(missing(CreateRequest { + env: &[("FOO", "bar")], + ..CreateRequest::shell(24, 80) + })); + } } diff --git a/crates/remote/src/lib.rs b/crates/remote/src/lib.rs index 2ff5b144..68dcf107 100644 --- a/crates/remote/src/lib.rs +++ b/crates/remote/src/lib.rs @@ -217,10 +217,20 @@ pub const C2S_SEARCH: u8 = 0x15; pub const C2S_CREATE_AT: u8 = 0x16; pub const C2S_CREATE_N: u8 = 0x17; /// Generic create: [0x18][nonce:2][rows:2][cols:2][features:1][tag_len:2][tag:N][...optional fields] -/// Features: bit 0 = has src_pty_id (2 bytes after tag), bit 1 = has command (remaining bytes after length-prefixed cwd if present), bit 2 = has cwd ([len:2][utf8]) +/// +/// Optional fields follow the tag in flag-bit order — src_pty, cwd, deadline, +/// env, argv — and the command, which has no length prefix, is always last. /// Server responds with S2C_CREATED_N using the same nonce. pub const C2S_CREATE2: u8 = 0x18; pub const CREATE2_HAS_SRC_PTY: u8 = 1 << 0; +/// Run this string through the server's login shell (`$SHELL -lic `): +/// the remaining bytes of the message, with no length prefix. +/// +/// **Legacy argv shape.** Before [`CREATE2_HAS_ARGV`] existed, a command +/// containing a NUL was split on NUL and exec'd directly, and every shipped +/// server still does that. It is lossy — empty arguments are dropped and the +/// payload is trimmed — so prefer `HAS_ARGV`, and reach for this only to talk +/// to a server that has not advertised [`FEATURE_CREATE_EXEC`]. pub const CREATE2_HAS_COMMAND: u8 = 1 << 1; pub const CREATE2_HAS_CWD: u8 = 1 << 2; /// Request exactly one correlated creation outcome: `S2C_CREATED_N` on @@ -238,6 +248,47 @@ pub const CREATE2_WANT_STATUS: u8 = 1 << 3; /// protecting it — a client that sends `C2S_DEADLINE` as a second message /// leaves the terminal unbounded if it dies in between. pub const CREATE2_HAS_DEADLINE: u8 = 1 << 4; +/// Environment overrides for the child: `[count:2]` then `count` records of +/// `[key_len:2][key:N][value_len:4][value:N]`, after any deadline and before +/// the command bytes. Entries are applied last, on top of everything the +/// server derives, so a client entry always wins. +/// +/// Only send this to a server advertising [`FEATURE_CREATE_EXEC`]. An older +/// one does not know bit 5, will not skip the block, and reads it as the +/// leading bytes of the command — running arbitrary garbage through the shell. +/// Same hazard as [`CREATE2_HAS_DEADLINE`], and worse in consequence. +pub const CREATE2_HAS_ENV: u8 = 1 << 5; +/// Exec the child directly instead of handing a string to the login shell: +/// `[argc:2]` then `argc` records of `[len:4][arg:N]`, after any environment +/// block and before the command bytes. Mutually exclusive with +/// [`CREATE2_HAS_COMMAND`]; a message carrying both is `INVALID`. +/// +/// Only send this to a server advertising [`FEATURE_CREATE_EXEC`]. An older +/// one does not know bit 6 and, finding no `HAS_COMMAND`, silently spawns the +/// default interactive shell instead of what was asked for. The compatible +/// spelling for such a server is the legacy shape — see [`CREATE2_HAS_COMMAND`] +/// and `docs/protocol.md`. +pub const CREATE2_HAS_ARGV: u8 = 1 << 6; + +/// Most arguments one `CREATE2` may carry. The whole exec block deliberately +/// reuses the process family's caps: it is the same `execve` at the other end, +/// and a second set of numbers would drift. +pub const CREATE2_MAX_ARGC: usize = process::PROCESS_MAX_ARGC; +/// Longest single argument, in bytes. +pub const CREATE2_MAX_ARG_LEN: usize = process::PROCESS_MAX_ARG_LEN; +/// Cap on the sum of all argument bytes. +pub const CREATE2_MAX_ARG_BYTES: usize = process::PROCESS_MAX_ARG_BYTES; +/// Most environment overrides one `CREATE2` may carry. +pub const CREATE2_MAX_ENVC: usize = process::PROCESS_MAX_ENVC; +/// Longest environment key, in bytes. +pub const CREATE2_MAX_ENV_KEY_LEN: usize = process::PROCESS_MAX_ENV_KEY_LEN; +/// Longest environment value, in bytes. +pub const CREATE2_MAX_ENV_VALUE_LEN: usize = process::PROCESS_MAX_ENV_VALUE_LEN; +/// Cap on the sum of all environment key and value bytes. +pub const CREATE2_MAX_ENV_BYTES: usize = process::PROCESS_MAX_ENV_BYTES; +/// Longest cwd accepted, in bytes. The field's own length prefix is a `u16`, +/// so this is what the shape can express rather than a policy choice. +pub const CREATE2_MAX_CWD_LEN: usize = u16::MAX as usize; /// Read text from a PTY's scrollback + viewport: [0x19][nonce:2][pty_id:2][offset:4][limit:4][flags:1] /// offset: number of lines to skip from the top (oldest = 0), or from the end if READ_TAIL is set /// limit: max lines to return (0 = all) @@ -994,6 +1045,15 @@ pub const FEATURE_CLIENT_CONTROL: u32 = 1 << 20; /// attempts. Implies [`FEATURE_CLIENT_CONTROL`]. pub const FEATURE_CLIENT_ORIGIN: u32 = 1 << 27; // Bit 28 is [`journal::FEATURE_TERM_JOURNAL`], with the rest of the family. +/// `C2S_CREATE2` accepts [`CREATE2_HAS_ARGV`] and [`CREATE2_HAS_ENV`], so a +/// terminal can be started the way a native process is: an exact argv exec'd +/// without a shell, plus environment overrides. +/// +/// Neither flag is probeable — an older server does not refuse an unknown +/// `CREATE2` bit, it ignores the bit and misreads the trailing bytes — so this +/// has to be advertised rather than discovered. Not advertised on Windows, +/// where the pseudoconsole path can honor neither. +pub const FEATURE_CREATE_EXEC: u32 = 1 << 29; #[derive(Clone, Copy, Debug, Default, PartialEq, Eq)] pub enum Color { @@ -3336,6 +3396,421 @@ pub fn msg_create2_full( msg } +/// Every optional field of a `C2S_CREATE2`, in one shape both ends agree on. +/// +/// The wire order is tag, `src_pty_id`, cwd, deadline, env, argv, command — +/// flag-bit order, with the unprefixed command last. `argv` and `command` are +/// mutually exclusive. +#[derive(Clone, Debug, Default, PartialEq, Eq)] +pub struct Create2Request<'a> { + pub nonce: u16, + pub rows: u16, + pub cols: u16, + /// Ask for one correlated outcome ([`CREATE2_WANT_STATUS`]). Only set this + /// against a server advertising [`FEATURE_CREATE_STATUS`]. + pub want_status: bool, + pub tag: &'a str, + /// Inherit the working directory of this terminal, when `cwd` is absent. + pub src_pty_id: Option, + pub cwd: Option<&'a str>, + pub deadline_ms: Option, + /// Environment overrides, applied on top of everything the server derives. + pub env: Vec<(&'a str, &'a str)>, + /// Exec this argv directly, no shell. On parse this is also where the + /// legacy NUL-split of a [`CREATE2_HAS_COMMAND`] payload lands. + pub argv: Option>, + /// Run this string through the server's login shell. + pub command: Option<&'a str>, +} + +/// Why a `C2S_CREATE2` was refused, shaped for the one-outcome contract. +/// +/// `nonce` is `None` only when the frame is too short to carry one; the server +/// must drop such a frame silently rather than answer an invented nonce +/// (docs/protocol.md, "Common status registry"). +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub struct Create2Error { + pub nonce: Option, + pub want_status: bool, + pub status: u8, + pub detail: &'static str, +} + +impl Create2Error { + fn at(nonce: u16, want_status: bool, status: u8, detail: &'static str) -> Self { + Self { + nonce: Some(nonce), + want_status, + status, + detail, + } + } +} + +/// Encode a [`Create2Request`]. +/// +/// Only set `argv`, `env`, or `deadline_ms` against a server advertising the +/// matching feature bit — see [`CREATE2_HAS_ARGV`], [`CREATE2_HAS_ENV`], and +/// [`CREATE2_HAS_DEADLINE`] for what an older one does with bytes it does not +/// know to skip. +/// +/// `command` is deliberately *not* checked for NULs: that is the legacy argv +/// spelling, and this encoder is how a client produces it on purpose. +pub fn msg_create2_request(req: &Create2Request<'_>) -> Result, Create2Error> { + let bad = |status, detail| Err(Create2Error::at(req.nonce, req.want_status, status, detail)); + if req.argv.is_some() && req.command.is_some() { + return bad(STATUS_INVALID, "argv and command are mutually exclusive"); + } + if let Some(argv) = &req.argv { + if argv.is_empty() { + return bad(STATUS_INVALID, "argv is empty"); + } + if argv.len() > CREATE2_MAX_ARGC { + return bad(STATUS_TOO_LARGE, "too many arguments"); + } + let mut total = 0usize; + for arg in argv { + if arg.len() > CREATE2_MAX_ARG_LEN { + return bad(STATUS_TOO_LARGE, "argument too long"); + } + total += arg.len(); + if total > CREATE2_MAX_ARG_BYTES { + return bad(STATUS_TOO_LARGE, "arguments too long in total"); + } + if arg.contains('\0') { + return bad(STATUS_INVALID, "argument contains a NUL"); + } + } + } + if let Some(detail) = env_violation(&req.env) { + return bad(detail.0, detail.1); + } + if req.tag.len() > u16::MAX as usize { + return bad(STATUS_TOO_LARGE, "tag too long"); + } + if req.cwd.is_some_and(|cwd| cwd.len() > CREATE2_MAX_CWD_LEN) { + return bad(STATUS_TOO_LARGE, "cwd too long"); + } + + let has_cwd = req.cwd.is_some_and(|cwd| !cwd.is_empty()); + let mut features = 0u8; + if req.src_pty_id.is_some() { + features |= CREATE2_HAS_SRC_PTY; + } + if has_cwd { + features |= CREATE2_HAS_CWD; + } + if req.want_status { + features |= CREATE2_WANT_STATUS; + } + if req.deadline_ms.is_some() { + features |= CREATE2_HAS_DEADLINE; + } + if !req.env.is_empty() { + features |= CREATE2_HAS_ENV; + } + if req.argv.is_some() { + features |= CREATE2_HAS_ARGV; + } + if req.command.is_some_and(|c| !c.is_empty()) { + features |= CREATE2_HAS_COMMAND; + } + + let mut msg = Vec::with_capacity(64 + req.tag.len()); + msg.push(C2S_CREATE2); + msg.extend_from_slice(&req.nonce.to_le_bytes()); + msg.extend_from_slice(&req.rows.to_le_bytes()); + msg.extend_from_slice(&req.cols.to_le_bytes()); + msg.push(features); + msg.extend_from_slice(&(req.tag.len() as u16).to_le_bytes()); + msg.extend_from_slice(req.tag.as_bytes()); + if let Some(src) = req.src_pty_id { + msg.extend_from_slice(&src.to_le_bytes()); + } + if has_cwd { + let cwd = req.cwd.unwrap_or_default().as_bytes(); + msg.extend_from_slice(&(cwd.len() as u16).to_le_bytes()); + msg.extend_from_slice(cwd); + } + if let Some(ms) = req.deadline_ms { + msg.extend_from_slice(&ms.to_le_bytes()); + } + if !req.env.is_empty() { + msg.extend_from_slice(&(req.env.len() as u16).to_le_bytes()); + for (key, value) in &req.env { + msg.extend_from_slice(&(key.len() as u16).to_le_bytes()); + msg.extend_from_slice(key.as_bytes()); + msg.extend_from_slice(&(value.len() as u32).to_le_bytes()); + msg.extend_from_slice(value.as_bytes()); + } + } + if let Some(argv) = &req.argv { + msg.extend_from_slice(&(argv.len() as u16).to_le_bytes()); + for arg in argv { + msg.extend_from_slice(&(arg.len() as u32).to_le_bytes()); + msg.extend_from_slice(arg.as_bytes()); + } + } + if let Some(command) = req.command.filter(|c| !c.is_empty()) { + msg.extend_from_slice(command.as_bytes()); + } + Ok(msg) +} + +/// Shared by the encoder and the parser so one set of rules governs both. +/// Mirrors `process::validate_spawn`: a key may not be empty, hold a NUL or an +/// `=`, or repeat, and a value may not hold a NUL. +fn env_violation(env: &[(&str, &str)]) -> Option<(u8, &'static str)> { + if env.len() > CREATE2_MAX_ENVC { + return Some((STATUS_TOO_LARGE, "too many environment entries")); + } + let mut total = 0usize; + for (i, (key, value)) in env.iter().enumerate() { + if key.len() > CREATE2_MAX_ENV_KEY_LEN { + return Some((STATUS_TOO_LARGE, "environment key too long")); + } + if value.len() > CREATE2_MAX_ENV_VALUE_LEN { + return Some((STATUS_TOO_LARGE, "environment value too long")); + } + total += key.len() + value.len(); + if total > CREATE2_MAX_ENV_BYTES { + return Some((STATUS_TOO_LARGE, "environment too large in total")); + } + if key.is_empty() { + return Some((STATUS_INVALID, "empty environment key")); + } + if key.contains('\0') || key.contains('=') || value.contains('\0') { + return Some((STATUS_INVALID, "environment entry contains NUL or '='")); + } + if env[..i].iter().any(|(prior, _)| prior == key) { + return Some((STATUS_INVALID, "duplicate environment key")); + } + } + None +} + +/// Decode a `C2S_CREATE2`, borrowing from `data`. +/// +/// The legacy fields keep their historical leniency — an undecodable cwd or +/// command is dropped rather than refused, because clients have always been +/// able to send one and having the terminal appear is the kinder answer. The +/// exec block is strict: it is new, so nothing depends on it being forgiving, +/// and silently exec'ing something other than what was asked is the one +/// outcome worth refusing outright. +pub fn parse_create2(data: &[u8]) -> Result, Create2Error> { + // Too short to carry a nonce and a feature byte: nothing to correlate a + // refusal to, so the caller drops it without answering. + if data.len() < 8 { + return Err(Create2Error { + nonce: None, + want_status: false, + status: STATUS_INVALID, + detail: "truncated create", + }); + } + let nonce = u16::from_le_bytes([data[1], data[2]]); + let rows = u16::from_le_bytes([data[3], data[4]]); + let cols = u16::from_le_bytes([data[5], data[6]]); + let features = data[7]; + let want_status = features & CREATE2_WANT_STATUS != 0; + let bad = |status, detail| Err(Create2Error::at(nonce, want_status, status, detail)); + + if data.len() < 10 { + return bad(STATUS_INVALID, "truncated tag length"); + } + let tag_len = u16::from_le_bytes([data[8], data[9]]) as usize; + let Some(tag_bytes) = data.get(10..10 + tag_len) else { + return bad(STATUS_INVALID, "tag length past end of message"); + }; + let Ok(tag) = std::str::from_utf8(tag_bytes) else { + return bad(STATUS_INVALID, "tag is not valid UTF-8"); + }; + let mut cursor = 10 + tag_len; + + // Historically honored only when the bytes are actually there; a short + // frame leaves it unset rather than refusing. + let src_pty_id = if features & CREATE2_HAS_SRC_PTY != 0 && data.len() >= cursor + 2 { + let id = u16::from_le_bytes([data[cursor], data[cursor + 1]]); + cursor += 2; + Some(id) + } else { + None + }; + + let cwd = if features & CREATE2_HAS_CWD != 0 { + if data.len() < cursor + 2 { + return bad(STATUS_INVALID, "truncated cwd length"); + } + let cwd_len = u16::from_le_bytes([data[cursor], data[cursor + 1]]) as usize; + cursor += 2; + if data.len() < cursor + cwd_len { + return bad(STATUS_INVALID, "truncated cwd"); + } + let cwd = std::str::from_utf8(&data[cursor..cursor + cwd_len]).ok(); + cursor += cwd_len; + cwd.filter(|p| !p.contains('\0')) + .map(str::trim) + .filter(|p| !p.is_empty()) + } else { + None + }; + + let deadline_ms = if features & CREATE2_HAS_DEADLINE != 0 { + if data.len() < cursor + 4 { + return bad(STATUS_INVALID, "truncated deadline"); + } + let ms = u32::from_le_bytes([ + data[cursor], + data[cursor + 1], + data[cursor + 2], + data[cursor + 3], + ]); + cursor += 4; + (ms > 0).then_some(ms) + } else { + None + }; + + let mut env: Vec<(&str, &str)> = Vec::new(); + if features & CREATE2_HAS_ENV != 0 { + if data.len() < cursor + 2 { + return bad(STATUS_INVALID, "truncated environment count"); + } + let count = u16::from_le_bytes([data[cursor], data[cursor + 1]]) as usize; + cursor += 2; + if count > CREATE2_MAX_ENVC { + return bad(STATUS_TOO_LARGE, "too many environment entries"); + } + env.reserve(count); + for _ in 0..count { + if data.len() < cursor + 2 { + return bad(STATUS_INVALID, "truncated environment key length"); + } + let key_len = u16::from_le_bytes([data[cursor], data[cursor + 1]]) as usize; + cursor += 2; + let Some(key) = data.get(cursor..cursor + key_len) else { + return bad(STATUS_INVALID, "truncated environment key"); + }; + cursor += key_len; + if data.len() < cursor + 4 { + return bad(STATUS_INVALID, "truncated environment value length"); + } + let value_len = u32::from_le_bytes([ + data[cursor], + data[cursor + 1], + data[cursor + 2], + data[cursor + 3], + ]) as usize; + cursor += 4; + if value_len > CREATE2_MAX_ENV_VALUE_LEN { + return bad(STATUS_TOO_LARGE, "environment value too long"); + } + let Some(value) = data.get(cursor..cursor + value_len) else { + return bad(STATUS_INVALID, "truncated environment value"); + }; + cursor += value_len; + let (Ok(key), Ok(value)) = (std::str::from_utf8(key), std::str::from_utf8(value)) + else { + return bad(STATUS_INVALID, "environment entry is not valid UTF-8"); + }; + env.push((key, value)); + } + if let Some((status, detail)) = env_violation(&env) { + return bad(status, detail); + } + } + + let mut argv: Option> = None; + if features & CREATE2_HAS_ARGV != 0 { + if features & CREATE2_HAS_COMMAND != 0 { + return bad(STATUS_INVALID, "argv and command are mutually exclusive"); + } + if data.len() < cursor + 2 { + return bad(STATUS_INVALID, "truncated argv count"); + } + let argc = u16::from_le_bytes([data[cursor], data[cursor + 1]]) as usize; + cursor += 2; + if argc > CREATE2_MAX_ARGC { + return bad(STATUS_TOO_LARGE, "too many arguments"); + } + if argc == 0 { + return bad(STATUS_INVALID, "argv is empty"); + } + let mut args = Vec::with_capacity(argc); + let mut total = 0usize; + for _ in 0..argc { + if data.len() < cursor + 4 { + return bad(STATUS_INVALID, "truncated argument length"); + } + let len = u32::from_le_bytes([ + data[cursor], + data[cursor + 1], + data[cursor + 2], + data[cursor + 3], + ]) as usize; + cursor += 4; + if len > CREATE2_MAX_ARG_LEN { + return bad(STATUS_TOO_LARGE, "argument too long"); + } + total += len; + if total > CREATE2_MAX_ARG_BYTES { + return bad(STATUS_TOO_LARGE, "arguments too long in total"); + } + let Some(arg) = data.get(cursor..cursor + len) else { + return bad(STATUS_INVALID, "truncated argument"); + }; + cursor += len; + let Ok(arg) = std::str::from_utf8(arg) else { + return bad(STATUS_INVALID, "argument is not valid UTF-8"); + }; + // A NUL cannot survive execve, so accepting one would be claiming + // to run something this can never run. + if arg.contains('\0') { + return bad(STATUS_INVALID, "argument contains a NUL"); + } + args.push(arg); + } + if args[0].is_empty() { + return bad(STATUS_INVALID, "argv[0] is empty"); + } + argv = Some(args); + } + + let mut command = None; + if features & CREATE2_HAS_COMMAND != 0 { + let payload = data.get(cursor..).and_then(|b| std::str::from_utf8(b).ok()); + // Legacy argv spelling: a payload carrying a NUL is a NUL-separated + // argv, not a shell string. Lossy — empty arguments vanish — which is + // exactly why `HAS_ARGV` exists. + if let Some(legacy) = payload + .filter(|p| p.contains('\0')) + .map(|p| p.split('\0').filter(|a| !a.is_empty()).collect::>()) + .filter(|args| !args.is_empty()) + { + argv = Some(legacy); + } else { + command = payload + .filter(|p| !p.contains('\0')) + .map(str::trim) + .filter(|p| !p.is_empty()); + } + } + + Ok(Create2Request { + nonce, + rows, + cols, + want_status, + tag, + src_pty_id, + cwd, + deadline_ms, + env, + argv, + command, + }) +} + pub fn msg_create_command(rows: u16, cols: u16, command: &str) -> Vec { msg_create_tagged_command(rows, cols, "", command) } @@ -6724,6 +7199,265 @@ mod tests { assert_eq!(armed.len(), plain.len() + 4); } + /// The struct encoder is the authority for the layout, so for every field + /// the eight-argument legacy encoder can also express, the two must agree + /// byte for byte — otherwise the parser is validated against one shape and + /// the shipping clients speak another. + #[test] + fn create2_struct_encoder_matches_the_legacy_encoder() { + for (cwd, deadline, want_status) in [ + (None, None, false), + (Some("/tmp"), None, false), + (None, Some(5_000), true), + (Some("/var/log"), Some(1), true), + ] { + let legacy = msg_create2_full( + 7, + 24, + 80, + "tag", + "echo hi", + if want_status { CREATE2_WANT_STATUS } else { 0 }, + cwd, + deadline, + ); + let structured = msg_create2_request(&Create2Request { + nonce: 7, + rows: 24, + cols: 80, + want_status, + tag: "tag", + cwd, + deadline_ms: deadline, + command: Some("echo hi"), + ..Default::default() + }) + .unwrap(); + assert_eq!(legacy, structured, "cwd={cwd:?} deadline={deadline:?}"); + } + } + + #[test] + fn create2_puts_argv_and_env_before_the_command() { + let msg = msg_create2_request(&Create2Request { + nonce: 1, + rows: 24, + cols: 80, + tag: "t", + cwd: Some("/tmp"), + deadline_ms: Some(5_000), + env: vec![("FOO", "bar")], + argv: Some(vec!["sleep", "60"]), + ..Default::default() + }) + .unwrap(); + assert_eq!(msg[0], C2S_CREATE2); + assert_ne!(msg[7] & CREATE2_HAS_ENV, 0); + assert_ne!(msg[7] & CREATE2_HAS_ARGV, 0); + assert_eq!(msg[7] & CREATE2_HAS_COMMAND, 0); + + let req = parse_create2(&msg).unwrap(); + assert_eq!(req.nonce, 1); + assert_eq!(req.tag, "t"); + assert_eq!(req.cwd, Some("/tmp")); + assert_eq!(req.deadline_ms, Some(5_000)); + assert_eq!(req.env, vec![("FOO", "bar")]); + assert_eq!(req.argv, Some(vec!["sleep", "60"])); + assert_eq!(req.command, None); + } + + #[test] + fn create2_argv_keeps_empty_arguments_the_legacy_shape_drops() { + let msg = msg_create2_request(&Create2Request { + argv: Some(vec!["sh", "-c", "", "x"]), + ..Default::default() + }) + .unwrap(); + assert_eq!( + parse_create2(&msg).unwrap().argv, + Some(vec!["sh", "-c", "", "x"]) + ); + + // The legacy NUL spelling of the same request cannot say it. + let legacy = msg_create2(0, 0, 0, "", "sh\0-c\0\0x", 0); + assert_eq!( + parse_create2(&legacy).unwrap().argv, + Some(vec!["sh", "-c", "x"]) + ); + } + + #[test] + fn create2_legacy_nul_payload_still_parses_as_argv() { + // Every shipped client spells argv this way; a one-word argv needs the + // trailing NUL to be distinguishable from a shell string. + let msg = msg_create2(3, 24, 80, "", "htop\0", 0); + let req = parse_create2(&msg).unwrap(); + assert_eq!(req.argv, Some(vec!["htop"])); + assert_eq!(req.command, None); + + let shell = msg_create2(3, 24, 80, "", "ls | wc -l", 0); + let req = parse_create2(&shell).unwrap(); + assert_eq!(req.argv, None); + assert_eq!(req.command, Some("ls | wc -l")); + } + + #[test] + fn create2_refuses_an_exec_request_it_cannot_honor() { + let reject = |req: Create2Request<'_>| { + msg_create2_request(&req) + .expect_err("encoder accepted an invalid request") + .status + }; + assert_eq!( + reject(Create2Request { + argv: Some(vec!["sh"]), + command: Some("sh"), + ..Default::default() + }), + STATUS_INVALID + ); + assert_eq!( + reject(Create2Request { + argv: Some(vec![]), + ..Default::default() + }), + STATUS_INVALID + ); + assert_eq!( + reject(Create2Request { + env: vec![("A", "1"), ("A", "2")], + ..Default::default() + }), + STATUS_INVALID + ); + assert_eq!( + reject(Create2Request { + env: vec![("A=B", "1")], + ..Default::default() + }), + STATUS_INVALID + ); + assert_eq!( + reject(Create2Request { + env: vec![("A", "x\0y")], + ..Default::default() + }), + STATUS_INVALID + ); + assert_eq!( + reject(Create2Request { + env: vec![("", "1")], + ..Default::default() + }), + STATUS_INVALID + ); + let many: Vec = (0..=CREATE2_MAX_ENVC).map(|i| format!("K{i}")).collect(); + assert_eq!( + reject(Create2Request { + env: many.iter().map(|k| (k.as_str(), "v")).collect(), + ..Default::default() + }), + STATUS_TOO_LARGE + ); + } + + /// Both blocks are length-prefixed, so every truncation has to be caught + /// rather than walked past — a cursor that under-advances turns the rest of + /// the frame into a command and runs something nobody asked for. + #[test] + fn create2_refuses_a_truncated_exec_block() { + let full = msg_create2_request(&Create2Request { + nonce: 9, + want_status: true, + env: vec![("FOO", "bar")], + argv: Some(vec!["sleep", "60"]), + ..Default::default() + }) + .unwrap(); + for cut in 10..full.len() { + let err = parse_create2(&full[..cut]) + .expect_err("a truncated exec block parsed as something runnable"); + assert_eq!(err.nonce, Some(9)); + assert!(err.want_status); + } + assert!(parse_create2(&full).is_ok()); + } + + /// Frames produced by `buildCreate2Message` in `@blit-sh/core`, pasted + /// verbatim. + /// + /// The layout has two independent encoders and one parser, and the risk is + /// asymmetric: this crate ships with the server, the TypeScript one ships + /// in every browser tab and every embedder that pinned an older release. + /// A change here that the JS side did not make would break them silently, + /// and nothing else in either test suite would notice — each one only ever + /// reads its own bytes back. Regenerate with the `buildCreate2Message` + /// tests in `js/core/src/__tests__/protocol.test.ts` if the layout moves + /// on purpose. + #[test] + fn frames_from_the_typescript_encoder_still_parse() { + fn hex(text: &str) -> Vec { + (0..text.len()) + .step_by(2) + .map(|i| u8::from_str_radix(&text[i..i + 2], 16).expect("hex")) + .collect() + } + + let argv_only = hex( + "18070018005000400000030005000000636172676f0400000074657374090000\ + 002d2d72656c65617365", + ); + let req = parse_create2(&argv_only).unwrap(); + assert_eq!((req.nonce, req.rows, req.cols), (7, 24, 80)); + assert_eq!( + req.argv.as_deref(), + Some(&["cargo", "test", "--release"][..]) + ); + assert_eq!(req.command, None); + + let env_only = hex( + "1807001800500020000002000800525553545f4c4f47050000006465627567\ + 0500454d50545900000000", + ); + let req = parse_create2(&env_only).unwrap(); + assert_eq!(req.env, vec![("RUST_LOG", "debug"), ("EMPTY", "")]); + + // Every optional field at once, which is the only case that proves the + // two encoders agree on field *order* rather than just on each block. + let everything = hex( + "180700180050007d05006275696c64030009002f7372632f626c697488130000\ + 02000800525553545f4c4f47050000006465627567040050415448080000002f\ + 6f70742f62696e0300020000007368020000002d6300000000", + ); + let req = parse_create2(&everything).unwrap(); + assert_eq!(req.tag, "build"); + assert_eq!(req.src_pty_id, Some(3)); + assert_eq!(req.cwd, Some("/src/blit")); + assert_eq!(req.deadline_ms, Some(5_000)); + assert_eq!(req.env, vec![("RUST_LOG", "debug"), ("PATH", "/opt/bin")]); + // The trailing empty argument is the one the legacy NUL spelling drops. + assert_eq!(req.argv.as_deref(), Some(&["sh", "-c", ""][..])); + assert!(req.want_status); + + let shell = hex("180700180050000e01007404002f746d706c73207c207763202d6c"); + let req = parse_create2(&shell).unwrap(); + assert_eq!((req.tag, req.cwd), ("t", Some("/tmp"))); + assert_eq!(req.command, Some("ls | wc -l")); + assert_eq!(req.argv, None); + } + + #[test] + fn create2_too_short_to_correlate_is_not_answerable() { + assert_eq!(parse_create2(&[C2S_CREATE2, 1, 0]).unwrap_err().nonce, None); + // Long enough for a nonce: refusals from here on can be correlated. + assert_eq!( + parse_create2(&[C2S_CREATE2, 1, 0, 24, 0, 80, 0, 0]) + .unwrap_err() + .nonce, + Some(1) + ); + } + #[test] fn exited_reason_roundtrips() { let wire = msg_exited_reason(3, -15, EXIT_REASON_DEADLINE); diff --git a/crates/server/src/lib.rs b/crates/server/src/lib.rs index 3485bbc6..6d02e920 100644 --- a/crates/server/src/lib.rs +++ b/crates/server/src/lib.rs @@ -35,8 +35,7 @@ use blit_remote::{ C2S_SURFACE_RESIZE, C2S_SURFACE_SUBSCRIBE, C2S_SURFACE_TEXT, C2S_SURFACE_TOUCH, C2S_SURFACE_UNSUBSCRIBE, C2S_TERM_CWD, C2S_UNSUBSCRIBE, CAPTURE_FORMAT_AVIF, CAPTURE_FORMAT_PNG, CLIENT_FEATURE_SURFACE_TIMESTAMP_SUB_US, CLIENT_LIST_WANT_ORIGIN, - CREATE2_HAS_COMMAND, CREATE2_HAS_CWD, CREATE2_HAS_DEADLINE, CREATE2_HAS_SRC_PTY, - CREATE2_WANT_STATUS, FEATURE_CLIENT_CONTROL, FEATURE_CLIENT_ORIGIN, FEATURE_COPY_RANGE, + FEATURE_CLIENT_CONTROL, FEATURE_CLIENT_ORIGIN, FEATURE_COPY_RANGE, FEATURE_CREATE_EXEC, FEATURE_CREATE_NONCE, FEATURE_CREATE_STATUS, FEATURE_KILL_MODE, FEATURE_PTY_DEADLINE, FEATURE_RESIZE_BATCH, FEATURE_RESTART, FEATURE_SCROLL_BY, FrameState, KICK_REASON_MAX, KILL_LEADER_ONLY, READ_ANSI, READ_TAIL, REMOTE_INPUT_POINTER, REMOTE_INPUT_TOUCH, S2C_CLOSED, @@ -2040,10 +2039,17 @@ struct Pty { /// Exit status: WEXITSTATUS if normal exit, negative signal number if signalled, /// EXIT_STATUS_UNKNOWN if not yet collected. exit_status: i32, - /// Command used to create this PTY (None = default shell). + /// The COMMAND column of `S2C_LIST`: what this terminal is running, as a + /// human reads it. `None` for a plain shell. For an argv terminal this + /// is a *rendering*, not something to run — restart reads `spec`. It is + /// computed by the create path rather than derived here, because the same + /// string has to clear `list_refusal`'s size guard before the terminal + /// exists, and a second rendering would be a second answer. command: Option, - /// Explicit working directory used to create this PTY. - cwd: Option, + /// What was actually started, replayed verbatim by `C2S_RESTART`. Without + /// this a terminal created with an argv, an environment, or both came back + /// from a restart as a bare login shell. + spec: pty::OwnedChildSpec, /// Working directory last reported by the shell via OSC 7, already /// validated by `parse_osc7_url` (docs/protocol.md, "Working directory /// tracking"). Last write wins; None until shell integration first @@ -6012,7 +6018,23 @@ fn reanchor_scrolled_clients(sess: &mut Session) { /// offset we hold for it right now — which is what makes the request immune /// to a re-anchor that crossed it on the wire. Unlike a re-anchor this may /// start a live client scrolling, since a wheel notch on a live view is how -/// scrolling back begins. Returns the new offset when it changed. +/// scrolling back begins. +/// +/// Returns the new offset only when the client has to be *told* it, which is +/// not the same as "when it changed". A relative request the client can +/// predict the outcome of needs no answer: it applied the same delta to the +/// same offset before it sent one. Answering anyway is actively wrong, and +/// wrong in a way that compounds — the answer is absolute, it arrives a round +/// trip late, and a wheel notch is several requests long, so by the time the +/// first lands the client has already moved past it. Adopting it drags the +/// view back, and the next delta, measured from the position it was dragged +/// back to, comes out too big. A twelve-row notch went out as 2, 2, 4, 4, 2 +/// and landed fourteen rows down; three notches landed forty rows instead of +/// thirty-six, with the view lurching the whole way. +/// +/// Clamping is the one outcome the client cannot predict — its own idea of the +/// scrollback's depth is a frame old and never counts the rows the same way — +/// so that is exactly when the answer is worth its round trip. fn scroll_client_by( client: &mut ClientState, pid: u16, @@ -6020,8 +6042,10 @@ fn scroll_client_by( max_offset: usize, ) -> Option { let current = client.scroll_offsets.get(&pid).copied().unwrap_or(0) as i64; - let next = current.saturating_add(delta).clamp(0, max_offset as i64) as usize; - update_client_scroll_state(client, pid, next).then_some(next) + let requested = current.saturating_add(delta); + let next = requested.clamp(0, max_offset as i64); + let changed = update_client_scroll_state(client, pid, next as usize); + (changed && requested != next).then_some(next as usize) } /// Move one client's parked view down by `delta` lines, bounded by the @@ -8895,22 +8919,86 @@ fn refuse_create( } } -/// Read a `CREATE2` tag out of `data`, or name why it is unusable. +/// Split a legacy create opcode's trailing payload into a shell command or an +/// argv, by the presence of a NUL. `C2S_CREATE` and `C2S_CREATE_N` predate +/// `CREATE2`'s explicit `HAS_ARGV`, and this is the only spelling they have. +fn legacy_create_payload(bytes: Option<&[u8]>) -> (Option<&str>, Option>) { + let payload = bytes.and_then(|bytes| std::str::from_utf8(bytes).ok()); + let argv = payload + .filter(|payload| payload.contains('\0')) + .map(|payload| { + payload + .split('\0') + .filter(|arg| !arg.is_empty()) + .collect::>() + }) + .filter(|args| !args.is_empty()); + if argv.is_some() { + return (None, argv); + } + let command = payload + .filter(|payload| !payload.contains('\0')) + .map(str::trim) + .filter(|payload| !payload.is_empty()); + (command, None) +} + +/// Longest rendering the COMMAND column will carry. +/// +/// `S2C_LIST` length-prefixes the field with a `u16`, and shell quoting can +/// quadruple a string, so a protocol-legal argv (up to a megabyte) has to be +/// bounded here or `list_refusal` starts rejecting creates for a reason no +/// client can predict from the advertised caps. Well past any real command +/// line, and far below the `u16` the encoder has to fit. +const MAX_LIST_COMMAND: usize = 4 * 1024; + +/// What `S2C_LIST` should show for a terminal, given how it was created. /// -/// `data` is the whole message; the tag is `[tag_len:2]` at offset 8 followed -/// by that many bytes. Both failures used to fall back to an empty tag and -/// let the create proceed, which breaks the one-outcome contract in two -/// different ways. A client correlating terminals by tag gets one it can -/// never match. Worse, an overrunning `tag_len` leaves the read cursor past -/// the end of the message, so a `CREATE2` carrying a command but no cwd or -/// deadline — nothing else left to bounds-check it — finds no command bytes -/// and spawns the default shell instead of what was asked for. -fn create2_tag(data: &[u8]) -> Result<&str, &'static str> { - let tag_len = u16::from_le_bytes([data[8], data[9]]) as usize; - let bytes = data - .get(10..10 + tag_len) - .ok_or("tag length past end of message")?; - std::str::from_utf8(bytes).map_err(|_| "tag is not valid UTF-8") +/// A shell command is shown as written. An argv is rendered the way a person +/// would type it, quoting only what has to be quoted, and elided if it runs +/// long. The result is display-only: `C2S_RESTART` replays `Pty::spec`, never +/// this, because feeding a rendering back through `sh -c` would silently swap +/// a direct exec for a login shell and drop the environment with it. +fn list_command(command: Option<&str>, argv: Option<&[&str]>) -> Option { + if let Some(command) = command { + return Some(command.to_owned()); + } + let argv = argv?; + let mut out = String::new(); + for arg in argv { + if !out.is_empty() { + out.push(' '); + } + if shell_safe(arg) { + out.push_str(arg); + } else { + out.push('\''); + out.push_str(&arg.replace('\'', r"'\''")); + out.push('\''); + } + if out.len() > MAX_LIST_COMMAND { + out.truncate( + // Never split a character in half; the field is UTF-8. + (0..=MAX_LIST_COMMAND) + .rev() + .find(|n| out.is_char_boundary(*n)) + .unwrap_or(0), + ); + out.push('…'); + break; + } + } + Some(out) +} + +/// Whether an argument survives a round trip through a shell unquoted. +/// Deliberately conservative — the cost of quoting something that did not need +/// it is cosmetic, the cost of the reverse is a misleading catalog. +fn shell_safe(arg: &str) -> bool { + !arg.is_empty() + && arg + .chars() + .all(|c| c.is_ascii_alphanumeric() || "@%_-+=:,./".contains(c)) } /// Name the field that would not survive `S2C_LIST`'s `u16` length prefixes, @@ -17957,6 +18045,12 @@ async fn handle_client_registered = None; - let create_payload = data - .get(cmd_start..) - .and_then(|bytes| std::str::from_utf8(bytes).ok()); - let command = create_payload - .filter(|payload| !payload.contains('\0')) - .map(str::trim) - .filter(|payload| !payload.is_empty()); - let argv: Option> = create_payload - .filter(|payload| payload.contains('\0')) - .map(|payload| { - payload - .split('\0') - .filter(|arg| !arg.is_empty()) - .collect::>() - }) - .filter(|args| !args.is_empty()); + let (command, argv) = legacy_create_payload(data.get(cmd_start..)); + let list_command = list_command(command, argv.as_deref()); // The legacy create opcodes carry no failure reply, so the // log is the only place a refusal can be said — as // `allocate_pty_id` already does for the cap. Refusing is // still necessary: `command` has no length prefix and runs to // the end of the frame, so #204's guard never covered it and an // oversize one truncates into a catalog every client misparses. - if let Some((_, detail)) = list_refusal(sess.pty_list_bytes(), tag, command) { + if let Some((_, detail)) = + list_refusal(sess.pty_list_bytes(), tag, list_command.as_deref()) + { eprintln!("blit-server: refusing CREATE, {detail}"); continue; } @@ -19802,9 +19884,13 @@ async fn handle_client_registered = None; - let create_payload = data - .get(cmd_start..) - .and_then(|bytes| std::str::from_utf8(bytes).ok()); - let command = create_payload - .filter(|payload| !payload.contains('\0')) - .map(str::trim) - .filter(|payload| !payload.is_empty()); - let argv: Option> = create_payload - .filter(|payload| payload.contains('\0')) - .map(|payload| { - payload - .split('\0') - .filter(|arg| !arg.is_empty()) - .collect::>() - }) - .filter(|args| !args.is_empty()); + let (command, argv) = legacy_create_payload(data.get(cmd_start..)); + let list_command = list_command(command, argv.as_deref()); // The legacy create opcodes carry no failure reply, so the // log is the only place a refusal can be said — as // `allocate_pty_id` already does for the cap. Refusing is // still necessary: `command` has no length prefix and runs to // the end of the frame, so #204's guard never covered it and an // oversize one truncates into a catalog every client misparses. - if let Some((_, detail)) = list_refusal(sess.pty_list_bytes(), tag, command) { + if let Some((_, detail)) = + list_refusal(sess.pty_list_bytes(), tag, list_command.as_deref()) + { eprintln!("blit-server: refusing CREATE, {detail}"); continue; } @@ -19908,9 +19982,13 @@ async fn handle_client_registered req, + Err(err) => { + if let Some(nonce) = err.nonce { + refuse_create( + &sess, + client_id, + err.want_status, + nonce, + err.status, + err.detail, + ); + } + continue; + } + }; + let nonce = req.nonce; + let want_status = req.want_status; + let tag = req.tag; // Straight off the wire and straight into the grid allocation. - let (rows, cols) = clamp_view_size( - u16::from_le_bytes([data[3], data[4]]), - u16::from_le_bytes([data[5], data[6]]), - ); - let features = data[7]; - let want_status = features & CREATE2_WANT_STATUS != 0; - if data.len() < 10 { + let (rows, cols) = clamp_view_size(req.rows, req.cols); + // The exec block is only honored where it can be honored; a + // host that would quietly run a login shell instead has to say + // no (docs/protocol.md, `FEATURE_CREATE_EXEC`). + if !cfg!(unix) && (!req.env.is_empty() || req.argv.is_some()) { refuse_create( &sess, client_id, want_status, nonce, STATUS_INVALID, - "truncated tag length", + "this host cannot exec an argv or override the environment", ); continue; } - let tag_len = u16::from_le_bytes([data[8], data[9]]) as usize; - let tag = match create2_tag(&data) { - Ok(tag) => tag, - Err(detail) => { - refuse_create(&sess, client_id, want_status, nonce, STATUS_INVALID, detail); - continue; - } - }; - let mut cursor = 10 + tag_len; - let src_dir = if features & CREATE2_HAS_SRC_PTY != 0 && data.len() >= cursor + 2 { - let src_id = u16::from_le_bytes([data[cursor], data[cursor + 1]]); - cursor += 2; - sess.ptys.get(&src_id).and_then(|p| pty::pty_cwd(&p.handle)) - } else { - None - }; - let explicit_dir = if features & CREATE2_HAS_CWD != 0 { - if data.len() < cursor + 2 { - refuse_create( - &sess, - client_id, - want_status, - nonce, - STATUS_INVALID, - "truncated cwd length", - ); - continue; - } - let cwd_len = u16::from_le_bytes([data[cursor], data[cursor + 1]]) as usize; - cursor += 2; - if data.len() < cursor + cwd_len { - refuse_create( - &sess, - client_id, - want_status, - nonce, - STATUS_INVALID, - "truncated cwd", - ); - continue; - } - let cwd = std::str::from_utf8(&data[cursor..cursor + cwd_len]).ok(); - cursor += cwd_len; - cwd.filter(|p| !p.contains('\0')) - .map(str::trim) - .filter(|p| !p.is_empty()) - .map(str::to_string) - } else { - None - }; - let dir = explicit_dir.or(src_dir); - // Before the command, which has no length prefix and runs to - // the end of the message. - let deadline_ms = if features & CREATE2_HAS_DEADLINE != 0 { - if data.len() < cursor + 4 { - refuse_create( - &sess, - client_id, - want_status, - nonce, - STATUS_INVALID, - "truncated deadline", - ); - continue; - } - let ms = u32::from_le_bytes([ - data[cursor], - data[cursor + 1], - data[cursor + 2], - data[cursor + 3], - ]); - cursor += 4; - (ms > 0).then_some(ms) - } else { - None - }; - let create_payload = if features & CREATE2_HAS_COMMAND != 0 { - data.get(cursor..).and_then(|b| std::str::from_utf8(b).ok()) - } else { - None - }; - let command = create_payload - .filter(|p| !p.contains('\0')) - .map(str::trim) - .filter(|p| !p.is_empty()); - let argv: Option> = create_payload - .filter(|p| p.contains('\0')) - .map(|p| p.split('\0').filter(|a| !a.is_empty()).collect::>()) - .filter(|a| !a.is_empty()); + let src_dir = req + .src_pty_id + .and_then(|id| sess.ptys.get(&id)) + .and_then(|p| pty::pty_cwd(&p.handle)); + let dir = req.cwd.map(str::to_owned).or(src_dir); + let deadline_ms = req.deadline_ms; + let command = req.command; + let argv = req.argv.clone(); + // Rendered once, here: `list_refusal` has to weigh exactly the + // string that will land in the catalog, or the guard passes on + // one length and `push_list_entry` truncates a different one. + let list_command = list_command(command, argv.as_deref()); // A record that cannot round-trip S2C_LIST's u16 length // fields, or a catalog that would outgrow what a client will // reassemble, means a corrupt or undeliverable frame for // everyone. Refuse the mutation instead. - if let Some((status, detail)) = list_refusal(sess.pty_list_bytes(), tag, command) { + if let Some((status, detail)) = + list_refusal(sess.pty_list_bytes(), tag, list_command.as_deref()) + { refuse_create(&sess, client_id, want_status, nonce, status, &detail); continue; } @@ -20194,9 +20212,17 @@ async fn handle_client_registered>(), + }, + list_command.as_deref(), config.scrollback, state.clone(), Some(&socket_name), @@ -21192,15 +21218,17 @@ async fn handle_client_registered= 3 => { let pid = u16::from_le_bytes([data[1], data[2]]); - let restart_info = sess.ptys.get(&pid).filter(|p| p.exited).map(|p| { - ( - p.driver.size(), - p.command.clone(), - p.cwd.clone(), - p.tag.clone(), - ) - }); - if let Some(((rows, cols), command, cwd, tag)) = restart_info { + // The whole spec, not just the command: a terminal started with + // an argv or an environment used to come back as a bare login + // shell, because those were the only two things restart could + // not see. + let restart_info = sess + .ptys + .get(&pid) + .filter(|p| p.exited) + .map(|p| (p.driver.size(), p.spec.clone(), p.tag.clone())); + if let Some(((rows, cols), spec, tag)) = restart_info { + let argv = spec.argv_refs(); let wayland_display = sess .compositor .as_ref() @@ -21219,8 +21247,7 @@ async fn handle_client_registered Vec { let mut msg = vec![0u8; 8]; + msg[0] = blit_remote::C2S_CREATE2; msg.extend_from_slice(&tag_len.to_le_bytes()); msg.extend_from_slice(tag); msg } + fn create2_tag(msg: &[u8]) -> Result<&str, &'static str> { + blit_remote::parse_create2(msg) + .map(|req| req.tag) + .map_err(|err| err.detail) + } + #[test] fn create2_tag_reads_a_well_formed_tag() { let msg = create2_with_tag(3, b"abc"); @@ -30536,6 +30593,64 @@ mod tests { assert_eq!(oversize_list_field(&exact, Some(&exact)), None); } + // ── the COMMAND column ── + + #[test] + fn list_command_shows_a_shell_command_as_written() { + assert_eq!( + list_command(Some("ls | wc -l"), None).as_deref(), + Some("ls | wc -l") + ); + assert_eq!(list_command(None, None), None); + } + + #[test] + fn list_command_renders_an_argv_the_way_it_would_be_typed() { + assert_eq!( + list_command(None, Some(&["ls", "-la", "/tmp"])).as_deref(), + Some("ls -la /tmp") + ); + // Anything a shell would eat gets quoted, including the empty argument + // the legacy NUL spelling could not carry at all. + assert_eq!( + list_command(None, Some(&["sh", "-c", "echo hi", ""])).as_deref(), + Some("sh -c 'echo hi' ''") + ); + assert_eq!( + list_command(None, Some(&["echo", "it's"])).as_deref(), + Some(r"echo 'it'\''s'") + ); + } + + /// The catalog's size guard runs against the string the create path renders + /// and `push_list_entry` writes the one a `Pty` stored. If a long argv could + /// slip past the guard and be stored anyway, `cmd.len() as u16` would + /// truncate the length prefix and desynchronize every client's catalog — + /// and `pty_list_bytes` would agree with the encoder the whole time, so the + /// `debug_assert` in `pty_list_msg` would never see it. + #[test] + fn list_command_stays_inside_what_the_catalog_can_encode() { + let huge = "x".repeat(blit_remote::CREATE2_MAX_ARG_LEN); + let argv: Vec<&str> = std::iter::repeat_n(huge.as_str(), 64).collect(); + let rendered = list_command(None, Some(&argv)).expect("argv renders"); + assert!( + rendered.len() <= MAX_LIST_COMMAND + 4, + "rendered {} bytes", + rendered.len() + ); + assert_eq!(oversize_list_field("tag", Some(&rendered)), None); + assert!(rendered.ends_with('…')); + } + + /// Quoting can multiply a string, so the elision has to be measured after + /// it, not before. + #[test] + fn list_command_elides_after_quoting_not_before() { + let quoted = "'".repeat(MAX_LIST_COMMAND); + let rendered = list_command(None, Some(&["echo", "ed])).expect("argv renders"); + assert!(rendered.len() <= MAX_LIST_COMMAND + 4); + } + // ── retention ── fn at(base: Instant, secs: u64) -> Instant { diff --git a/crates/server/src/pty/pty_unix.rs b/crates/server/src/pty/pty_unix.rs index f27f1f12..38c9dbfe 100644 --- a/crates/server/src/pty/pty_unix.rs +++ b/crates/server/src/pty/pty_unix.rs @@ -6,10 +6,77 @@ use tokio::sync::{Notify, mpsc}; use crate::{AppState, PTY_CHANNEL_CAPACITY, PtyInput}; +/// What to run in a terminal, and where. +/// +/// `command` and `argv` are mutually exclusive: a command is handed to the +/// login shell, an argv is exec'd directly. Both are absent for a plain shell. +/// The owned counterpart, [`OwnedChildSpec`], is what a `Pty` keeps so a +/// restart can replay the same child rather than degrading to a bare shell. +#[derive(Clone, Copy, Debug, Default)] +pub struct ChildSpec<'a> { + /// Run through `$SHELL -c`. + pub command: Option<&'a str>, + /// Exec directly, no shell. + pub argv: Option<&'a [&'a str]>, + pub dir: Option<&'a str>, + /// Environment overrides, applied after everything the server derives. + pub env: &'a [(String, String)], +} + +/// [`ChildSpec`] with owned strings, held by a `Pty` for the lifetime of the +/// terminal so `C2S_RESTART` re-runs what was actually started. +#[derive(Clone, Debug, Default, PartialEq, Eq)] +pub struct OwnedChildSpec { + pub command: Option, + pub argv: Option>, + pub dir: Option, + pub env: Vec<(String, String)>, +} + +impl OwnedChildSpec { + pub fn borrowed<'a>(&'a self, argv: &'a [&'a str]) -> ChildSpec<'a> { + ChildSpec { + command: self.command.as_deref(), + argv: self.argv.is_some().then_some(argv), + dir: self.dir.as_deref(), + env: &self.env, + } + } + + /// The `&[&str]` backing store `borrowed` needs, since `Vec` and + /// `&[&str]` have no shared layout. + pub fn argv_refs(&self) -> Vec<&str> { + self.argv + .as_deref() + .map(|args| args.iter().map(String::as_str).collect()) + .unwrap_or_default() + } +} + +impl ChildSpec<'_> { + pub fn to_owned_spec(self) -> OwnedChildSpec { + OwnedChildSpec { + command: self.command.map(str::to_owned), + argv: self + .argv + .map(|args| args.iter().map(|a| (*a).to_owned()).collect()), + dir: self.dir.map(str::to_owned), + env: self.env.to_vec(), + } + } +} + /// Build the environment array for a child process before fork(). /// This avoids calling std::env::set_var/remove_var after fork() in a /// multi-threaded process (which is UB per POSIX — those functions are /// not async-signal-safe). +/// +/// `overrides` are applied **dead last**, after the inherit filter, after the +/// terminal and `BLIT_*` rewrites, and after the session environment — so a +/// client entry always wins, whichever layer would otherwise have set the key. +/// This is the precedence the process family already documents for +/// `PROCESS_SPAWN` (`command_for` in `process.rs`). +#[allow(clippy::too_many_arguments)] fn build_child_env( wayland_display: Option<&str>, x_display: Option<&str>, @@ -18,6 +85,7 @@ fn build_child_env( pipewire_remote: Option<&str>, blit_sock: Option<&str>, path_dir: Option<&str>, + overrides: &[(String, String)], ) -> Vec { let mut env: Vec<(String, String)> = std::env::vars() .filter(|(k, _)| { @@ -80,11 +148,122 @@ fn build_child_env( for (key, value) in &session.set { set(&mut env, key, value); } + // Last word to the client, over every layer above — including the `BLIT_*` + // filter and the exported socket, which a caller may legitimately want to + // point somewhere else. + for (key, value) in overrides { + set(&mut env, key, value); + } env.into_iter() .filter_map(|(k, v)| CString::new(format!("{k}={v}")).ok()) .collect() } +/// Everything the child needs to `execve`, built entirely before `fork()`. +/// +/// Nothing here may be deferred to the child: after fork in a multi-threaded +/// process only async-signal-safe calls are legal, and every allocation risks +/// an allocator mutex some dead thread still holds. That includes the +/// `CString`s — a NUL in a client-supplied argument must fail *here*, not as a +/// panic on the wrong side of the fork. +struct ExecPlan { + /// Kept alive because `ptrs` borrows their interiors. + _argv: Vec, + program: CString, + ptrs: Vec<*const libc::c_char>, +} + +impl ExecPlan { + /// `program` is what runs; `args` is the child's whole argv, argv[0] + /// included. The two are allowed to disagree, and both callers make them: + /// `program` is resolved against the child's own `PATH`, while argv[0] + /// stays as it was written — the client's word for it, or the shell's + /// name — so `ps` and busybox-style dispatch see the request rather than + /// the path it resolved to. + fn new(program: &std::path::Path, args: &[&str]) -> Option { + let program = CString::new(program.as_os_str().as_encoded_bytes()).ok()?; + let argv: Vec = args + .iter() + .map(|arg| CString::new(*arg).ok()) + .collect::>()?; + let ptrs = argv + .iter() + .map(|arg| arg.as_ptr()) + .chain(std::iter::once(std::ptr::null())) + .collect(); + Some(Self { + _argv: argv, + program, + ptrs, + }) + } + + /// Only `execve` and `_exit` run after this; both are async-signal-safe. + unsafe fn exec(&self, envp: &[*const libc::c_char]) -> ! { + unsafe { + libc::execve(self.program.as_ptr(), self.ptrs.as_ptr(), envp.as_ptr()); + libc::_exit(1); + } + } +} + +/// Resolve and lay out the child's `execve` arguments before forking. +/// +/// `env` is the child's own environment, so `PATH` lookup honors an override +/// the caller asked for rather than silently using the server's. +fn plan_exec( + spec: &ChildSpec<'_>, + shell: &str, + shell_flags: &str, + env: &[CString], +) -> Option { + if let Some(argv) = spec.argv.filter(|argv| !argv.is_empty()) { + let program = resolve_in_path(argv[0], child_path(env).as_deref())?; + return ExecPlan::new(&program, argv); + } + let program = resolve_in_path(shell, child_path(env).as_deref()) + .unwrap_or_else(|| std::path::PathBuf::from(shell)); + let flag = match (spec.command, shell_flags) { + (Some(_), "") => Some("-c".to_owned()), + (Some(_), flags) => Some(format!("-{flags}c")), + (None, "") => None, + (None, flags) => Some(format!("-{flags}")), + }; + let mut args: Vec<&str> = vec![shell]; + if let Some(flag) = &flag { + args.push(flag); + } + if let Some(command) = spec.command { + args.push(command); + } + ExecPlan::new(&program, &args) +} + +/// The `PATH` the child will actually run with, read back out of its own +/// prepared environment. +fn child_path(env: &[CString]) -> Option { + env.iter().find_map(|entry| { + entry + .to_str() + .ok() + .and_then(|entry| entry.strip_prefix("PATH=")) + .map(str::to_owned) + }) +} + +/// Write a diagnostic to the terminal and terminate the child. +/// +/// Runs after fork, so it is restricted to `write` and `_exit`. The message +/// reaches the pty, which is the only place a person is looking. +unsafe fn child_fail(what: &[u8], detail: &[u8]) -> ! { + unsafe { + for part in [b"blit: " as &[u8], what, detail, b"\r\n"] { + libc::write(2, part.as_ptr().cast(), part.len()); + } + libc::_exit(1); + } +} + /// Directory holding the running server binary, resolved once. `None` when the /// path can't be read or has no usable parent. fn exe_dir() -> Option<&'static str> { @@ -96,13 +275,25 @@ fn exe_dir() -> Option<&'static str> { .as_deref() } -/// Resolve a program name to an absolute path by searching $PATH. +/// Resolve a program name to an absolute path by searching `$PATH`. /// Called before fork() so the child can use execve (which doesn't search PATH). -fn resolve_in_path(program: &str) -> Option { +/// +/// `path` is the child's own `PATH` when the caller has one — an override that +/// changes where a program comes from has to change where we look for it, or +/// the terminal runs a different binary than the same command would in a shell. +/// Falls back to the server's. +fn resolve_in_path(program: &str, path: Option<&str>) -> Option { if program.contains('/') { return Some(std::path::PathBuf::from(program)); } - let path_var = std::env::var("PATH").unwrap_or_default(); + let owned; + let path_var = match path { + Some(path) => path, + None => { + owned = std::env::var("PATH").unwrap_or_default(); + &owned + } + }; for dir in path_var.split(':') { let candidate = std::path::Path::new(dir).join(program); if candidate.is_file() { @@ -601,6 +792,11 @@ pub fn pty_reader(fd: PtyWriteTarget, tx: mpsc::Sender, notify: Arc, - argv: Option<&[&str]>, - dir: Option<&str>, + spec: ChildSpec<'_>, + list_command: Option<&str>, scrollback: usize, state: AppState, wayland_display: Option<&str>, @@ -658,14 +853,36 @@ pub fn spawn_pty( pipewire_remote, blit_sock, path_dir, + spec.env, ); let child_envp: Vec<*const libc::c_char> = child_env .iter() .map(|c| c.as_ptr()) .chain(std::iter::once(std::ptr::null())) .collect(); - // Resolve the shell path before fork (execve doesn't search PATH). - let shell_path = resolve_in_path(shell); + // Resolve the program and lay out its argv before fork: execve does not + // search PATH, and neither the allocation nor a NUL-check may happen on + // the child's side of the fork. + let Some(plan) = plan_exec(&spec, shell, shell_flags, &child_env) else { + eprintln!("cannot resolve a program to run for pty {id}"); + unsafe { + libc::close(master); + libc::close(slave); + } + return None; + }; + let dir_c = match spec.dir.map(CString::new) { + Some(Ok(dir)) => Some(dir), + None => None, + Some(Err(_)) => { + eprintln!("working directory for pty {id} contains a NUL"); + unsafe { + libc::close(master); + libc::close(slave); + } + return None; + } + }; let pid = fork_child(); if pid < 0 { @@ -698,67 +915,15 @@ pub fn spawn_pty( libc::signal(libc::SIGPIPE, libc::SIG_DFL); } set_qos_user_interactive(); - let effective_dir = dir.map(String::from); - if let Some(d) = effective_dir - && let Ok(dir_c) = CString::new(d) - { - unsafe { - libc::chdir(dir_c.as_ptr()); - } - } - if let Some(command) = command { - let shell_c = match &shell_path { - Some(p) => CString::new(p.to_string_lossy().as_ref()).unwrap(), - None => CString::new(shell).unwrap(), - }; - let command_c = CString::new(command).unwrap(); - let flag = CString::new(if shell_flags.is_empty() { - "-c".to_owned() - } else { - format!("-{}c", shell_flags) - }) - .unwrap(); - unsafe { - let p = shell_c.as_ptr(); - let f = flag.as_ptr(); - let c = command_c.as_ptr(); - libc::execve(p, [p, f, c, std::ptr::null()].as_ptr(), child_envp.as_ptr()); - libc::_exit(1); - } - } - if let Some(args) = argv - && !args.is_empty() + // A working directory that cannot be entered used to be ignored, which + // left the child running somewhere the client never asked for and had + // no way to notice. Say so on the terminal and stop. + if let Some(dir_c) = &dir_c + && unsafe { libc::chdir(dir_c.as_ptr()) } != 0 { - let cargs: Vec = args.iter().map(|s| CString::new(*s).unwrap()).collect(); - // Resolve the first arg (program) via PATH. - let prog = resolve_in_path(args[0]) - .map(|p| CString::new(p.to_string_lossy().as_ref()).unwrap()) - .unwrap_or_else(|| cargs[0].clone()); - let ptrs: Vec<*const libc::c_char> = std::iter::once(prog.as_ptr()) - .chain(cargs[1..].iter().map(|c| c.as_ptr())) - .chain(std::iter::once(std::ptr::null())) - .collect(); - unsafe { - libc::execve(prog.as_ptr(), ptrs.as_ptr(), child_envp.as_ptr()); - libc::_exit(1); - } - } - let shell_c = match &shell_path { - Some(p) => CString::new(p.to_string_lossy().as_ref()).unwrap(), - None => CString::new(shell).unwrap(), - }; - unsafe { - if shell_flags.is_empty() { - let p = shell_c.as_ptr(); - libc::execve(p, [p, std::ptr::null()].as_ptr(), child_envp.as_ptr()); - } else { - let flag = CString::new(format!("-{}", shell_flags)).unwrap(); - let p = shell_c.as_ptr(); - let f = flag.as_ptr(); - libc::execve(p, [p, f, std::ptr::null()].as_ptr(), child_envp.as_ptr()); - } - libc::_exit(1); + unsafe { child_fail(b"cannot enter working directory: ", dir_c.as_bytes()) }; } + unsafe { plan.exec(&child_envp) } } unsafe { @@ -807,8 +972,8 @@ pub fn spawn_pty( exited_at: None, generation: 0, exit_status: blit_remote::EXIT_STATUS_UNKNOWN, - command: command.map(|s| s.to_owned()), - cwd: dir.map(|s| s.to_owned()), + command: list_command.map(str::to_owned), + spec: spec.to_owned_spec(), osc7_cwd: None, journal: crate::journal::CommandJournal::default(), osc_carry: Vec::new(), @@ -822,8 +987,7 @@ pub fn respawn_child( rows: u16, cols: u16, pty_id: u16, - command: Option<&str>, - dir: Option<&str>, + spec: ChildSpec<'_>, state: AppState, wayland_display: Option<&str>, x_display: Option<&str>, @@ -871,13 +1035,19 @@ pub fn respawn_child( pipewire_remote, blit_sock, path_dir, + spec.env, ); let child_envp: Vec<*const libc::c_char> = child_env .iter() .map(|c| c.as_ptr()) .chain(std::iter::once(std::ptr::null())) .collect(); - let shell_path = resolve_in_path(shell); + let plan = plan_exec(&spec, shell, shell_flags, &child_env)?; + let dir_c = match spec.dir.map(CString::new) { + Some(Ok(dir)) => Some(dir), + None => None, + Some(Err(_)) => return None, + }; let pid = fork_child(); if pid < 0 { @@ -902,56 +1072,12 @@ pub fn respawn_child( libc::signal(libc::SIGPIPE, libc::SIG_DFL); } set_qos_user_interactive(); - if let Some(d) = dir - && let Ok(dir_c) = CString::new(d) + if let Some(dir_c) = &dir_c + && unsafe { libc::chdir(dir_c.as_ptr()) } != 0 { - unsafe { - libc::chdir(dir_c.as_ptr()); - } - } - if let Some(cmd) = command { - let shell_c = match &shell_path { - Some(p) => CString::new(p.to_string_lossy().as_ref()).unwrap(), - None => CString::new(shell).unwrap(), - }; - let flag = CString::new(if shell_flags.is_empty() { - "-c".to_owned() - } else { - format!("-{}c", shell_flags) - }) - .unwrap(); - let cmd_c = CString::new(cmd).unwrap(); - unsafe { - libc::execve( - shell_c.as_ptr(), - [ - shell_c.as_ptr(), - flag.as_ptr(), - cmd_c.as_ptr(), - std::ptr::null(), - ] - .as_ptr(), - child_envp.as_ptr(), - ); - libc::_exit(1); - } - } - let shell_c = match &shell_path { - Some(p) => CString::new(p.to_string_lossy().as_ref()).unwrap(), - None => CString::new(shell).unwrap(), - }; - unsafe { - if shell_flags.is_empty() { - let p = shell_c.as_ptr(); - libc::execve(p, [p, std::ptr::null()].as_ptr(), child_envp.as_ptr()); - } else { - let flag = CString::new(format!("-{}", shell_flags)).unwrap(); - let p = shell_c.as_ptr(); - let f = flag.as_ptr(); - libc::execve(p, [p, f, std::ptr::null()].as_ptr(), child_envp.as_ptr()); - } - libc::_exit(1); + unsafe { child_fail(b"cannot enter working directory: ", dir_c.as_bytes()) }; } + unsafe { plan.exec(&child_envp) } } unsafe { @@ -979,10 +1105,38 @@ pub fn respawn_child( #[cfg(test)] mod tests { - use super::{PtyHandle, build_child_env, collect_exit_status, reap_zombies}; + use super::{ + ChildSpec, PtyHandle, build_child_env, child_path, collect_exit_status, plan_exec, + reap_zombies, resolve_in_path, + }; use std::collections::HashMap; + use std::ffi::CString; use std::time::{Duration, Instant}; + /// `build_child_env` with no client overrides — the shape every test that + /// predates them expects. + #[allow(clippy::too_many_arguments)] + fn session_child_env( + wayland_display: Option<&str>, + x_display: Option<&str>, + desktop_bus: Option<&str>, + pulse_server: Option<&str>, + pipewire_remote: Option<&str>, + blit_sock: Option<&str>, + path_dir: Option<&str>, + ) -> Vec { + build_child_env( + wayland_display, + x_display, + desktop_bus, + pulse_server, + pipewire_remote, + blit_sock, + path_dir, + &[], + ) + } + /// Block until `pid` exits but leave it unreaped (`WNOWAIT`), so the reaper /// under test still finds a zombie to consume. fn wait_until_zombie(pid: libc::pid_t) { @@ -1289,9 +1443,123 @@ mod tests { .collect() } + fn overrides(pairs: &[(&str, &str)]) -> Vec<(String, String)> { + pairs + .iter() + .map(|(k, v)| ((*k).to_string(), (*v).to_string())) + .collect() + } + + /// The client's entries are applied after every layer the server derives — + /// the inherit filter, the terminal rewrites, the exported socket, and the + /// session environment — so "explicit beats inherited" holds no matter + /// which layer would otherwise have owned the key. #[test] - fn child_env_enables_electron_wayland_when_compositor_is_available() { + fn child_env_overrides_beat_every_layer_the_server_derives() { let env = child_env_map(build_child_env( + Some("/tmp/blit-test/wayland-7"), + None, + None, + None, + None, + Some("/tmp/blit-test/ipc.sock"), + None, + &overrides(&[ + // A plain addition. + ("BLIT_PROBE", "hello"), + // Beats the unconditional terminal rewrite. + ("TERM", "dumb"), + // Beats the exported socket. + ("BLIT_SOCK", "/somewhere/else.sock"), + // Beats `session_env`'s compositor socket. + ("WAYLAND_DISPLAY", "wayland-99"), + ]), + )); + assert_eq!(env.get("BLIT_PROBE").map(String::as_str), Some("hello")); + assert_eq!(env.get("TERM").map(String::as_str), Some("dumb")); + assert_eq!( + env.get("BLIT_SOCK").map(String::as_str), + Some("/somewhere/else.sock") + ); + assert_eq!( + env.get("WAYLAND_DISPLAY").map(String::as_str), + Some("wayland-99") + ); + } + + /// An override that changes where programs come from has to change where + /// we look for them, or the terminal runs a different binary than the same + /// command would in a shell. + #[test] + fn path_lookup_follows_the_child_environment() { + let dir = std::env::temp_dir().join(format!("blit-path-{}", std::process::id())); + std::fs::create_dir_all(&dir).unwrap(); + let program = dir.join("blit-test-probe"); + std::fs::write(&program, b"#!/bin/sh\n").unwrap(); + + let env = build_child_env( + None, + None, + None, + None, + None, + None, + None, + &overrides(&[("PATH", dir.to_str().unwrap())]), + ); + assert_eq!(child_path(&env).as_deref(), dir.to_str()); + assert_eq!( + resolve_in_path("blit-test-probe", child_path(&env).as_deref()), + Some(program.clone()) + ); + // Without it the server's own PATH answers, and this is not on it. + assert_eq!(resolve_in_path("blit-test-probe", None), None); + + std::fs::remove_dir_all(&dir).ok(); + } + + /// A NUL cannot survive `execve`. It used to reach a `CString::new().unwrap()` + /// on the child's side of a `fork` in a multi-threaded process, where a + /// panic is neither async-signal-safe nor recoverable. + #[test] + fn a_nul_in_an_argument_fails_before_the_fork() { + let argv = ["echo", "a\0b"]; + assert!( + plan_exec( + &ChildSpec { + argv: Some(&argv), + ..Default::default() + }, + "/bin/sh", + "", + &[], + ) + .is_none() + ); + } + + /// A program that does not exist is a failure to plan, not a fork that + /// exits 1 with nothing said. + #[test] + fn an_unresolvable_program_fails_before_the_fork() { + let argv = ["blit-definitely-not-a-program"]; + assert!( + plan_exec( + &ChildSpec { + argv: Some(&argv), + ..Default::default() + }, + "/bin/sh", + "", + &[], + ) + .is_none() + ); + } + + #[test] + fn child_env_enables_electron_wayland_when_compositor_is_available() { + let env = child_env_map(session_child_env( Some("/tmp/blit-test/wayland-7"), None, None, @@ -1338,7 +1606,7 @@ mod tests { /// it to run at all. #[test] fn child_env_exports_display_only_for_a_bridged_session() { - let env = child_env_map(build_child_env( + let env = child_env_map(session_child_env( Some("/tmp/blit-test/wayland-7"), Some(":20"), None, @@ -1355,7 +1623,7 @@ mod tests { // No compositor, no session to point at: DISPLAY stays gone even // when the host had one. - let env = child_env_map(build_child_env( + let env = child_env_map(session_child_env( None, Some(":20"), None, @@ -1369,7 +1637,7 @@ mod tests { #[test] fn child_env_uses_the_compositor_scoped_session_bus() { - let env = child_env_map(build_child_env( + let env = child_env_map(session_child_env( Some("/tmp/blit-test/wayland-7"), None, None, @@ -1380,7 +1648,7 @@ mod tests { )); assert!(!env.contains_key("DBUS_SESSION_BUS_ADDRESS")); - let env = child_env_map(build_child_env( + let env = child_env_map(session_child_env( Some("/tmp/blit-test/wayland-7"), None, Some("unix:path=/tmp/blit-test/desktop-bus"), @@ -1397,10 +1665,10 @@ mod tests { #[test] fn child_env_exports_blit_sock_only_when_requested() { - let env = child_env_map(build_child_env(None, None, None, None, None, None, None)); + let env = child_env_map(session_child_env(None, None, None, None, None, None, None)); assert!(!env.contains_key("BLIT_SOCK")); - let env = child_env_map(build_child_env( + let env = child_env_map(session_child_env( None, None, None, @@ -1419,13 +1687,13 @@ mod tests { fn child_env_appends_the_binary_dir_to_path_only_when_requested() { let inherited = std::env::var("PATH").unwrap_or_default(); - let env = child_env_map(build_child_env(None, None, None, None, None, None, None)); + let env = child_env_map(session_child_env(None, None, None, None, None, None, None)); assert_eq!( env.get("PATH").map(String::as_str), Some(inherited.as_str()) ); - let env = child_env_map(build_child_env( + let env = child_env_map(session_child_env( None, None, None, @@ -1445,7 +1713,7 @@ mod tests { let inherited = std::env::var("PATH").unwrap_or_default(); let already = inherited.split(':').next_back().unwrap_or_default(); - let env = child_env_map(build_child_env( + let env = child_env_map(session_child_env( None, None, None, diff --git a/crates/server/src/pty/pty_windows.rs b/crates/server/src/pty/pty_windows.rs index 23d8d550..23bde5f4 100644 --- a/crates/server/src/pty/pty_windows.rs +++ b/crates/server/src/pty/pty_windows.rs @@ -337,6 +337,61 @@ fn build_command_line(shell: &str, shell_flags: &str, command: Option<&str>) -> to_wide(&cmd) } +/// What to run in a terminal, and where. +/// +/// The pseudoconsole takes a command *line*, so `argv` and `env` are carried +/// for signature parity with the Unix path and cannot be honored here — which +/// is why `FEATURE_CREATE_EXEC` is not advertised on this platform and the +/// create path refuses a request that sets either. +#[derive(Clone, Copy, Debug, Default)] +pub struct ChildSpec<'a> { + pub command: Option<&'a str>, + pub argv: Option<&'a [&'a str]>, + pub dir: Option<&'a str>, + pub env: &'a [(String, String)], +} + +/// [`ChildSpec`] with owned strings, held by a `Pty` so a restart can replay +/// the same child. +#[derive(Clone, Debug, Default, PartialEq, Eq)] +pub struct OwnedChildSpec { + pub command: Option, + pub argv: Option>, + pub dir: Option, + pub env: Vec<(String, String)>, +} + +impl OwnedChildSpec { + pub fn borrowed<'a>(&'a self, argv: &'a [&'a str]) -> ChildSpec<'a> { + ChildSpec { + command: self.command.as_deref(), + argv: self.argv.is_some().then_some(argv), + dir: self.dir.as_deref(), + env: &self.env, + } + } + + pub fn argv_refs(&self) -> Vec<&str> { + self.argv + .as_deref() + .map(|args| args.iter().map(String::as_str).collect()) + .unwrap_or_default() + } +} + +impl ChildSpec<'_> { + pub fn to_owned_spec(self) -> OwnedChildSpec { + OwnedChildSpec { + command: self.command.map(str::to_owned), + argv: self + .argv + .map(|args| args.iter().map(|a| (*a).to_owned()).collect()), + dir: self.dir.map(str::to_owned), + env: self.env.to_vec(), + } + } +} + #[allow(clippy::too_many_arguments)] pub fn spawn_pty( shell: &str, @@ -345,9 +400,8 @@ pub fn spawn_pty( cols: u16, id: u16, tag: &str, - command: Option<&str>, - _argv: Option<&[&str]>, - dir: Option<&str>, + spec: ChildSpec<'_>, + list_command: Option<&str>, scrollback: usize, state: AppState, _wayland_display: Option<&str>, @@ -356,6 +410,8 @@ pub fn spawn_pty( _pulse_server: Option<&str>, _pipewire_remote: Option<&str>, ) -> Option { + let command = spec.command; + let dir = spec.dir; let (input_read, input_write) = create_pipe_pair()?; let (output_read, output_write) = create_pipe_pair()?; @@ -508,8 +564,8 @@ pub fn spawn_pty( exited_at: None, generation: 0, exit_status: blit_remote::EXIT_STATUS_UNKNOWN, - command: command.map(|s| s.to_owned()), - cwd: dir.map(|s| s.to_owned()), + command: list_command.map(str::to_owned), + spec: spec.to_owned_spec(), osc7_cwd: None, journal: crate::journal::CommandJournal::default(), osc_carry: Vec::new(), @@ -522,8 +578,7 @@ pub fn respawn_child( rows: u16, cols: u16, pty_id: u16, - command: Option<&str>, - dir: Option<&str>, + spec: ChildSpec<'_>, state: AppState, _wayland_display: Option<&str>, _x_display: Option<&str>, @@ -535,6 +590,8 @@ pub fn respawn_child( std::thread::JoinHandle<()>, mpsc::Receiver, )> { + let command = spec.command; + let dir = spec.dir; let (input_read, input_write) = create_pipe_pair()?; let (output_read, output_write) = create_pipe_pair()?; diff --git a/docs/protocol.md b/docs/protocol.md index acfa55b7..621184b0 100644 --- a/docs/protocol.md +++ b/docs/protocol.md @@ -54,7 +54,7 @@ every pending operation as a connection error in that case. | `0x13` | `SUBSCRIBE` | `[pty_id:2]` | | `0x14` | `UNSUBSCRIBE` | `[pty_id:2]` | | `0x15` | `SEARCH` | `[request_id:2][query:N]` | -| `0x16` | `CREATE_AT` | `[rows:2][cols:2][src_pty_id:2][tag_len:2][tag:N]` | +| `0x16` | `CREATE_AT` | `[rows:2][cols:2][tag_len:2][tag:N][src_pty_id:2]` | | `0x17` | `CREATE_N` | `[nonce:2][rows:2][cols:2][tag_len:2][tag:N]` | | `0x18` | `CREATE2` | `[nonce:2][rows:2][cols:2][features:1][tag_len:2][tag:N][optional…]` | | `0x19` | `READ` | `[nonce:2][pty_id:2][offset:4][limit:4][flags:1]` | @@ -129,6 +129,18 @@ viewer receives a downscaled stream at its requested physical size. - Bit 2 (`HAS_CWD`): followed by `[cwd_len:2][cwd:N]` (before any command bytes) — spawn in this working directory. - Bit 3 (`WANT_STATUS`): valid only when `HELLO` advertises `CREATE_STATUS`; requests one correlated `CREATED_N` or `CREATE_FAILED` outcome. It adds no trailing field. - Bit 4 (`HAS_DEADLINE`): followed by `[ms:4]`, after any cwd and before any command bytes — arm a deadline at creation. Valid only when `HELLO` advertises `PTY_DEADLINE`. +- Bit 5 (`HAS_ENV`): followed by `[count:2]` then `count` records of `[key_len:2][key:N][value_len:4][value:N]` — environment overrides for the child. Valid only when `HELLO` advertises `CREATE_EXEC`. +- Bit 6 (`HAS_ARGV`): followed by `[argc:2]` then `argc` records of `[len:4][arg:N]` — exec this argv directly, no shell. Mutually exclusive with `HAS_COMMAND`; a message setting both is `INVALID`. Valid only when `HELLO` advertises `CREATE_EXEC`. + +Optional fields appear in flag-bit order — `src_pty_id`, cwd, deadline, env, argv — and the command, which has no length prefix, is always last. + +Every field-bearing bit past `HAS_CWD` is unsafe against a server that does not advertise it. An older server does not refuse an unknown `features` bit; it ignores the bit, does not skip the field, and reads those bytes as the start of the command. `HAS_ARGV` fails differently and just as quietly: with no `HAS_COMMAND` set, the server spawns the default interactive shell. Negotiate, do not probe. + +Environment entries are applied last, after everything the server derives for a terminal — the inherited environment, `TERM`, an exported `BLIT_SOCK`, and the session variables — so a client entry always wins. Keys may not be empty, hold a NUL or an `=`, or repeat; values may not hold a NUL. Limits match the process family: 1024 arguments and 1 MiB of argument bytes, 256 variables and 1 MiB of key and value bytes, 64 KiB for any single argument or value, 255 bytes for a key. + +**Legacy argv.** Before `HAS_ARGV`, a `HAS_COMMAND` payload containing a NUL was split on NUL and exec'd directly, and every server still accepts that spelling. It is lossy — empty arguments are dropped and the payload is trimmed — so it exists only to reach a server without `CREATE_EXEC`. A one-argument command needs a trailing NUL to be distinguishable from a shell string. + +The COMMAND column of `S2C_LIST` shows an argv terminal a shell-quoted rendering of its argv, elided if it runs long. That rendering is for display: `RESTART` replays what the terminal was actually created with, argv and environment included. `READ` requests text from a PTY's scrollback + viewport: @@ -327,7 +339,7 @@ shared sizing input without a `SURFACE_RESIZE` entry. | `0x2A` | `SURFACE_ENCODER` | `[surface_id:2][name][0x00][codec_string]` — encoder display name + WebCodecs codec string, NUL-separated | | `0x2B` | `FRAGMENT` | `[flags:1][chunk:N]` — see [Fragmentation](#fragmentation) | | `0x2C` | `CLIPBOARD_LIST` | `[count:2] repeated{ [mime_len:2][mime:N] }` | -| `0x2D` | `SURFACE_ACTIVATED` | `[surface_id:2]` — the Wayland client asked for its toplevel to be activated (xdg_activation_v1); raise and focus the pane | +| `0x2D` | `SURFACE_ACTIVATED` | `[surface_id:2]` — the Wayland client asked for its toplevel to be activated (xdg_activation_v1). Highlight the surface where it already is; do **not** raise it or take focus. Tokens are cheap and unacknowledged, so clients repeat this several times a second, and each would land after whatever the viewer just chose | | `0x2E` | `CLIPBOARD_OWNER` | `[wayland:1]` — `1` while a Wayland client owns the selection; `0` when empty or externally owned | | `0x2F` | `SURFACE_TEXT_INPUT` | `[surface_id:2][flags:1][content_hint:4][content_purpose:4]` — committed `zwp_text_input_v3` state; flags bit 0 is enabled and bit 1 marks a fresh enable request; optional `[cursor_x:2i][cursor_y:2i][cursor_w:2i][cursor_h:2i]` caret tail | | `0x30` | `AUDIO_FRAME` | `[timestamp:4][flags:1][data:N]` | @@ -386,6 +398,7 @@ shared sizing input without a `SURFACE_RESIZE` entry. | 26 | `CHANNEL_WATCH` | `CHANNEL_WATCH` follows which channel names have a listener | | 27 | `CLIENT_ORIGIN` | The client catalog can say which connections are extensions | | 28 | `TERM_JOURNAL` | Per-command journal and sequence-addressed output | +| 29 | `CREATE_EXEC` | `CREATE2(HAS_ARGV)` and `CREATE2(HAS_ENV)`; Unix hosts only | Bit 26 is advertised with bit 12 and never alone; it is separate because a `WATCH` an older server does not know is dropped by the channel family's diff --git a/docs/server.md b/docs/server.md index d252f950..805a0aaa 100644 --- a/docs/server.md +++ b/docs/server.md @@ -101,6 +101,14 @@ with the same key. Clients cannot clear the inherited environment. Unix PTY creation separately rewrites terminal and compositor integration variables; those PTY-only rewrites do not apply to native pipe children. +Terminals accept the same environment overrides, through +`CREATE2(HAS_ENV)` on a server advertising `CREATE_EXEC`. They are applied +after every rewrite above, so a client entry wins over the terminal variables, +an exported `BLIT_SOCK`, and the session variables alike. As with processes, +there is no way to clear the inherited environment — only to replace entries in +it. `PATH` is honored where it matters: an override changes where the server +looks for the program it is about to exec. + ## PTY lifecycle ### Creation @@ -108,14 +116,19 @@ those PTY-only rewrites do not apply to native pipe children. PTYs are created by `C2S_CREATE` or `C2S_CREATE2`. The server: 1. Allocates a PTY pair via `openpty`. -2. Forks. The child sets the slave fd as controlling terminal (`TIOCSCTTY`), closes inherited descriptors except stdio, sets the working directory, and `exec`s the shell (or custom command from `HAS_COMMAND`). +2. Resolves the program, lays out its `argv`, and builds the child environment — all **before** the fork, since only async-signal-safe calls are legal after it. A NUL in a client-supplied argument, or a program that cannot be found, fails the create here rather than in the child. +3. Forks. The child sets the slave fd as controlling terminal (`TIOCSCTTY`), closes inherited descriptors except stdio, enters the working directory, and `exec`s. It runs the `argv` from `HAS_ARGV` directly, or hands the string from `HAS_COMMAND` to the login shell, or starts the default shell. A working directory it cannot enter is reported on the terminal and the child exits, rather than silently running somewhere else. The child runs as the **same user as the server** — there is no `setuid`, `setgid`, `chroot`, or seccomp anywhere in the tree. Closing descriptors keeps one terminal from reaching another's PTY master or the IPC listener; it is hygiene between sibling terminals, not a boundary between a client and the machine. A blit connection is equivalent to an interactive login shell as the server's user; confinement, if you need it, belongs outside the server (see the `fd-channel` integration point in [transports.md](transports.md)). -3. The master fd is registered with the tokio reactor for async I/O. -4. PTY output is fed through the `blit-alacritty` terminal parser. -5. `S2C_CREATED` (or `S2C_CREATED_N` with nonce) is sent to the creating client. -6. All connected clients receive `S2C_LIST` reflecting the new PTY. +4. The master fd is registered with the tokio reactor for async I/O. +5. PTY output is fed through the `blit-alacritty` terminal parser. +6. `S2C_CREATED` (or `S2C_CREATED_N` with nonce) is sent to the creating client. +7. All connected clients receive `S2C_LIST` reflecting the new PTY. + +The terminal remembers what it was created with — command or `argv`, working +directory, and environment overrides — so `C2S_RESTART` re-runs the same child +rather than falling back to a bare login shell. ### Exit diff --git a/e2e/tests/clients-extension.spec.ts b/e2e/tests/clients-extension.spec.ts index 2288708a..a1508ef1 100644 --- a/e2e/tests/clients-extension.spec.ts +++ b/e2e/tests/clients-extension.spec.ts @@ -3,6 +3,27 @@ import { execFileSync } from "child_process"; import fs from "fs"; import path from "path"; +/** + * A manage tile is registered in the *host's* open-tab list (docs/design/kv.md), + * so leaving one open outlives this page: it is the first parked card in every + * spec that runs after this one, and their own `localStorage.clear()` cannot + * reach it — a card whose body is a title rather than a preview, which is not + * what a dock full of terminals is expected to start with. Closing the focused + * tile is the only thing that unregisters it. + */ +test.afterEach(async ({ page }) => { + const panels = page.locator("[data-connection-tab]"); + if ( + !(await panels + .first() + .isVisible() + .catch(() => false)) + ) + return; + await page.keyboard.press("Control+Alt+Shift+q"); + await expect(panels).toHaveCount(0); +}); + /** * An extension is a client, and the clients list now says so. * @@ -86,7 +107,7 @@ test("the clients list names the extension behind a connection", async ({ expect(installed).toMatch(/^id:[0-9a-f]+$/); await page.getByRole("status").click(); - const manage = page.getByRole("button", { name: /Manage/ }).first(); + const manage = page.getByRole("button", { name: /^Manage$/ }).first(); await expect(manage).toBeVisible({ timeout: 5_000 }); await manage.click(); await page.locator('[data-connection-tab="clients"]').click(); diff --git a/e2e/tests/extension-tabs.spec.ts b/e2e/tests/extension-tabs.spec.ts index ce87ce1e..f3b08a84 100644 --- a/e2e/tests/extension-tabs.spec.ts +++ b/e2e/tests/extension-tabs.spec.ts @@ -3,6 +3,27 @@ import { execFileSync } from "child_process"; import fs from "fs"; import path from "path"; +/** + * A manage tile is registered in the *host's* open-tab list (docs/design/kv.md), + * so leaving one open outlives this page: it is the first parked card in every + * spec that runs after this one, and their own `localStorage.clear()` cannot + * reach it — a card whose body is a title rather than a preview, which is not + * what a dock full of terminals is expected to start with. Closing the focused + * tile is the only thing that unregisters it. + */ +test.afterEach(async ({ page }) => { + const panels = page.locator("[data-connection-tab]"); + if ( + !(await panels + .first() + .isVisible() + .catch(() => false)) + ) + return; + await page.keyboard.press("Control+Alt+Shift+q"); + await expect(panels).toHaveCount(0); +}); + /** * A remote's extension tabs appear and disappear with the extension. * @@ -95,7 +116,7 @@ test("installing an extension adds its tab, removing it takes it away", async ({ ).toBeVisible({ timeout: 10_000 }); await page.getByRole("status").click(); - const manage = page.getByRole("button", { name: /Manage/ }).first(); + const manage = page.getByRole("button", { name: /^Manage$/ }).first(); await expect(manage).toBeVisible({ timeout: 5_000 }); await manage.click(); await expect(page.locator("[data-connection-tab]").first()).toBeVisible({ diff --git a/e2e/tests/journal-live.spec.ts b/e2e/tests/journal-live.spec.ts index 54221bfc..be65cbe3 100644 --- a/e2e/tests/journal-live.spec.ts +++ b/e2e/tests/journal-live.spec.ts @@ -1,6 +1,27 @@ import { test, expect } from "@playwright/test"; import { execFileSync } from "node:child_process"; +/** + * A manage tile is registered in the *host's* open-tab list (docs/design/kv.md), + * so leaving one open outlives this page: it is the first parked card in every + * spec that runs after this one, and their own `localStorage.clear()` cannot + * reach it — a card whose body is a title rather than a preview, which is not + * what a dock full of terminals is expected to start with. Closing the focused + * tile is the only thing that unregisters it. + */ +test.afterEach(async ({ page }) => { + const panels = page.locator("[data-connection-tab]"); + if ( + !(await panels + .first() + .isVisible() + .catch(() => false)) + ) + return; + await page.keyboard.press("Control+Alt+Shift+q"); + await expect(panels).toHaveCount(0); +}); + /** * The journal pane is a live tail over history that grows as it is scrolled. * @@ -27,7 +48,7 @@ test("the journal tails live and pages history in as it scrolls", async ({ ).toBeVisible({ timeout: 10_000 }); await page.getByRole("status").click(); - const manage = page.getByRole("button", { name: /Manage/ }).first(); + const manage = page.getByRole("button", { name: /^Manage$/ }).first(); await expect(manage).toBeVisible({ timeout: 5_000 }); await manage.click(); diff --git a/e2e/tests/remote-panels.spec.ts b/e2e/tests/remote-panels.spec.ts index 3879aff0..b76465b9 100644 --- a/e2e/tests/remote-panels.spec.ts +++ b/e2e/tests/remote-panels.spec.ts @@ -1,15 +1,42 @@ import { test, expect } from "@playwright/test"; /** - * Everything a remote has to say lives under that remote. + * A manage tile is registered in the *host's* open-tab list (docs/design/kv.md), + * so leaving one open outlives this page: it is the first parked card in every + * spec that runs after this one, and their own `localStorage.clear()` cannot + * reach it — a card whose body is a title rather than a preview, which is not + * what a dock full of terminals is expected to start with. Closing the focused + * tile is the only thing that unregisters it. + */ +test.afterEach(async ({ page }) => { + const panels = page.locator("[data-connection-tab]"); + if ( + !(await panels + .first() + .isVisible() + .catch(() => false)) + ) + return; + await page.keyboard.press("Control+Alt+Shift+q"); + await expect(panels).toHaveCount(0); +}); + +/** + * Everything a remote has to say lives under that remote — as a pane. * * systemd units and extensions used to be status-bar glyphs opening overlays * of their own, which put them next to the font size and the audio mute — * workspace chrome for things that are properties of one server. They are now - * tabs of one remote's Manage panel, alongside its applications and clients. - * This asserts both halves: the glyphs are gone, and the tabs are there. + * tabs of one remote's Manage tile, alongside its applications and clients. + * + * And a tile rather than a dialog because a dialog could not survive being + * used: enabling an application in the Session tab starts it, a fresh window + * asks to be raised, and an activation closes whatever overlay is up. So this + * asserts three things: the glyphs are gone, Manage opens pane content (the + * remotes dialog closing behind it), and the panels are still there after the + * click that used to dismiss them. */ -test("a remote's panels open from its Manage button, not from status-bar glyphs", async ({ +test("a remote's panels open as a pane from its Manage button, not from status-bar glyphs", async ({ page, }) => { await page.goto("/"); @@ -38,14 +65,17 @@ test("a remote's panels open from its Manage button, not from status-bar glyphs" timeout: 5_000, }); - // One connected remote opens its own management overlay. - const control = page.getByRole("button", { name: /Manage/ }).first(); + // Exactly "Manage", not merely containing it: a parked manage tile's dock + // card is a button too, and its accessible name starts with the remote's + // name and ends with the word — which a /Manage/ locator matches, under a + // modal backdrop, unclickably. + const control = page.getByRole("button", { name: /^Manage$/ }).first(); await expect(control).toBeVisible({ timeout: 5_000 }); await control.click(); - // Its own dialog, on top of the remotes list rather than inside a row. - await expect( - page.locator('[role="dialog"][aria-label^="Manage"]'), - ).toHaveCount(1); + + // Pane content, and the dialog that asked for it is gone rather than + // stacked under a second one. + await expect(page.locator('[role="dialog"]')).toHaveCount(0); // Clients is the tab every connected server can offer; the extension-backed // ones appear only where their channel answers, so this asserts the strip @@ -54,6 +84,14 @@ test("a remote's panels open from its Manage button, not from status-bar glyphs" await expect(tabs.first()).toBeVisible({ timeout: 5_000 }); await expect(page.locator('[data-connection-tab="clients"]')).toHaveCount(1); + // The bar says which pane is focused for every other kind of tile, and a + // manage tile publishes the same two halves its dock card carries: the + // address, then the tab that is up. Read from the bar's own region so a + // match on the tab strip behind it cannot pass for one here. + const identity = page.locator("[data-status-identity]"); + await expect(identity).toContainText(/:manage/, { timeout: 5_000 }); + await expect(identity).toContainText("Clients"); + // Extensions is a server capability rather than an installed extension, so // it is present here, and its registry defaults to the dev stack's own — // three ports up from the page, which is what bin/dev allocates. @@ -73,13 +111,13 @@ test("a remote's panels open from its Manage button, not from status-bar glyphs" ), ); - // Escape closes the management panel and leaves the remotes list standing: - // one key, one layer. - await page.keyboard.press("Escape"); - await expect( - page.locator('[role="dialog"][aria-label^="Manage"]'), - ).toHaveCount(0); - await expect( - page.getByRole("button", { name: /Manage/ }).first(), - ).toBeVisible(); + // A tile is addressed by the URL like every other one, so a reload brings + // the panels back rather than the workspace it replaced. + await expect + .poll(() => page.evaluate(() => location.hash), { timeout: 5_000 }) + .toMatch(/tile=/); + await page.reload(); + await expect(page.locator("[data-connection-tab]").first()).toBeVisible({ + timeout: 10_000, + }); }); diff --git a/js/core/src/BlitConnection.ts b/js/core/src/BlitConnection.ts index 7517035d..488abab7 100644 --- a/js/core/src/BlitConnection.ts +++ b/js/core/src/BlitConnection.ts @@ -21,6 +21,8 @@ import { FEATURE_CLIENT_ORIGIN, FEATURE_CREATE_NONCE, FEATURE_CREATE_STATUS, + FEATURE_CREATE_EXEC, + FEATURE_PTY_DEADLINE, FEATURE_KILL_MODE, FEATURE_RESIZE_BATCH, FEATURE_SCROLL_BY, @@ -490,10 +492,24 @@ export interface CreateSessionOptions { rows: number; cols: number; tag?: string; + /** Run this through the server's login shell. Mutually exclusive with + * {@link argv}. */ command?: string; + /** Exec this argv directly — no login shell, so no rc files and no shell + * syntax. Rejected unless the server advertised `FEATURE_CREATE_EXEC`, + * because an older one would quietly start a plain shell instead. */ + argv?: readonly string[]; cwdFromSessionId?: SessionId; /** Working directory for the new session. Interpreted on the target server. */ cwd?: string; + /** Environment overrides for the child, applied on top of everything the + * server derives. Rejected unless the server advertised + * `FEATURE_CREATE_EXEC`, because an older one would drop them silently. */ + env?: Readonly>; + /** Stop the terminal server-side after this many milliseconds, armed at + * creation so it survives this client dying. Rejected unless the server + * advertised `FEATURE_PTY_DEADLINE`. */ + deadlineMs?: number; } type ResizeSessionOptions = { @@ -1196,6 +1212,7 @@ export class BlitConnection { supportsChannels: false, supportsChannelWatch: false, supportsExtensions: false, + supportsCreateExec: false, supportsDesktopMedia: false, retryCount: 0, bootGeneration: null, @@ -1397,6 +1414,19 @@ export class BlitConnection { `Cannot create PTY while transport is ${this.transport.status}`, ); } + // Refuse rather than send a field this server would misread. An unknown + // CREATE2 flag is not rejected by the server — it is ignored, and the + // bytes behind it are read as something else — so the only safe check is + // this one, here, before anything goes out. + const hasEnv = Object.keys(options.env ?? {}).length > 0; + if ((options.argv || hasEnv) && !(this.features & FEATURE_CREATE_EXEC)) { + throw connectionError( + "Server does not support starting a terminal with an explicit argv or environment", + ); + } + if (options.deadlineMs != null && !(this.features & FEATURE_PTY_DEADLINE)) { + throw connectionError("Server does not support terminal deadlines"); + } return new Promise((resolve, reject) => { let nonce = 0; @@ -1413,14 +1443,17 @@ export class BlitConnection { this.pendingCreates.set(nonce, { resolve, reject, - command: options.command, + command: options.command ?? options.argv?.join(" "), }); this.transport.send( buildCreate2Message(nonce, options.rows, options.cols, { tag: options.tag, command: options.command, + argv: options.argv, srcPtyId, cwd: options.cwd, + env: options.env, + deadlineMs: options.deadlineMs, wantStatus: (this.features & FEATURE_CREATE_STATUS) !== 0, }), ); @@ -4673,9 +4706,26 @@ export class BlitConnection { private surfaceViewIdCounter = 0; /** Allocate a token identifying one view's subscription to a surface. - * Mirrors {@link allocViewId} for PTYs. */ + * Mirrors {@link allocViewId} for PTYs. + * + * Prefixed with the connection id, because a view mints its token once and + * keeps it for the life of its mount — including across + * `BlitSurfaceCanvas.setConnectionId`, which re-points a canvas at another + * server without re-minting. A bare per-connection counter is only unique + * within the connection that issued it, so a canvas carrying `s3` from + * connection A onto connection B collided with B's own `s3`: two views + * sharing one `SurfaceSub.views` entry, where the last writer decides the + * encode size and cadence for both. A live pane then inherited a dock + * card's `{512x256, 15fps}` request and could not take it back + * (`serverSubscribe` early-returns once `_subscribedSurface` is set), and + * the card's unsubscribe deleted the pane's registration outright, taking + * the pane's stream with it. + * + * The token never reaches the wire — it only keys {@link surfaceSubs}' + * `views` and {@link surfaceViewSizes}' `views` — so the prefix costs + * nothing but the string. Session ids are built the same way. */ allocSurfaceViewId(): string { - return `s${++this.surfaceViewIdCounter}`; + return `${this.id}:s${++this.surfaceViewIdCounter}`; } /** @@ -6023,6 +6073,7 @@ export class BlitConnection { supportsChannelWatch: (features & FEATURE_CHANNEL_WATCH) !== 0, supportsExtensions: (features & FEATURE_EXTENSION) !== 0, supportsDesktopMedia: (features & FEATURE_DESKTOP_MEDIA) !== 0, + supportsCreateExec: (features & FEATURE_CREATE_EXEC) !== 0, bootGeneration, serverVersion, }; diff --git a/js/core/src/BlitSurfaceCanvas.ts b/js/core/src/BlitSurfaceCanvas.ts index 57f1b411..7e2df42f 100644 --- a/js/core/src/BlitSurfaceCanvas.ts +++ b/js/core/src/BlitSurfaceCanvas.ts @@ -717,6 +717,67 @@ const surfaceCanvasByInput = new WeakMap< BlitSurfaceCanvas >(); +/** Where the canvas is on screen and how its pixels map to surface pixels. + * Obtained from a `getBoundingClientRect`, so treat it as a measurement. */ +interface DrawnGeometry { + dx: number; + dy: number; + dw: number; + dh: number; + sx: number; + sy: number; + rect: DOMRect; +} + +/** + * Bumped whenever anything might have moved a canvas on screen without + * changing its own box: a window resize, a scroll in any ancestor, a + * visual-viewport change (mobile keyboard). + * + * This exists so {@link BlitSurfaceCanvas.syncImeTarget} can skip its + * `getBoundingClientRect` on the overwhelming majority of frames. It used to + * measure on *every presented frame* — the only notification a pane being + * dragged gives us — which put a forced layout, four visual-viewport reads and + * a `position: fixed` style write inside the decoder's present path at up to + * the display's refresh rate, for the one pane the user has focused. Those + * writes then invalidated layout for the wheel handler's own reads, which is + * the read/write thrash that made scrolling a focused pane expensive. + * + * One shared counter and one shared set of listeners, refcounted across + * mounts: a per-view listener would put the cost back, multiplied by the number + * of dock cards on the page. + */ +let layoutEpoch = 0; +let layoutEpochRefs = 0; +let layoutEpochListener: (() => void) | null = null; + +function retainLayoutEpoch(): void { + layoutEpochRefs++; + if (layoutEpochListener || typeof window === "undefined") return; + layoutEpochListener = () => { + layoutEpoch++; + }; + // Capture, so a scroll in any ancestor of any surface pane counts — scroll + // does not bubble. Passive: these never call preventDefault. + window.addEventListener("scroll", layoutEpochListener, { + capture: true, + passive: true, + }); + window.addEventListener("resize", layoutEpochListener, { passive: true }); + window.visualViewport?.addEventListener("resize", layoutEpochListener); + window.visualViewport?.addEventListener("scroll", layoutEpochListener); +} + +function releaseLayoutEpoch(): void { + layoutEpochRefs = Math.max(0, layoutEpochRefs - 1); + if (layoutEpochRefs > 0 || !layoutEpochListener) return; + window.removeEventListener("scroll", layoutEpochListener, { capture: true }); + window.removeEventListener("resize", layoutEpochListener); + window.visualViewport?.removeEventListener("resize", layoutEpochListener); + window.visualViewport?.removeEventListener("scroll", layoutEpochListener); + layoutEpochListener = null; +} + /** Bubbling DOM event emitted by a mounted surface when its Wayland client * commits text-input state. The app shell uses fresh `requested` events to * raise a mobile virtual keyboard; embedders can provide their own policy. */ @@ -1380,6 +1441,15 @@ export class BlitSurfaceCanvas { w: number; h: number; } | null = null; + /** {@link layoutEpoch} the IME capture element was last placed against, or + * -1 to force the next {@link syncImeTarget} to measure. */ + private _imeSyncedEpoch = -1; + /** Whether the pointer overlay already carries the fill-the-box style the + * no-display-size branch of {@link layoutCanvasBox} writes. */ + private _overlayFilled = false; + /** Whether this mount holds a reference on the shared {@link layoutEpoch} + * listeners. */ + private _layoutEpochHeld = false; /** True after this view has sent a nonzero surface resize that must be * cleared when the view stops owning foreground/BSP sizing. */ private _resizeConstraintActive = false; @@ -1693,6 +1763,12 @@ export class BlitSurfaceCanvas { mountedSurfaceCanvases.set(canvas, this); this.observePresentBox(container); + // Flagged rather than counted per call: attach() has no re-entrancy guard, + // and a double retain would strand the shared listeners for the page's life. + if (!this._layoutEpochHeld) { + this._layoutEpochHeld = true; + retainLayoutEpoch(); + } this.observeIntersection(container); this.subscribe(); this.attachEvents(); @@ -1800,6 +1876,10 @@ export class BlitSurfaceCanvas { dispose(): void { if (this.disposed) return; this.disposed = true; + if (this._layoutEpochHeld) { + this._layoutEpochHeld = false; + releaseLayoutEpoch(); + } if (this._retryUnsub) { this._retryUnsub(); this._retryUnsub = undefined; @@ -2107,7 +2187,12 @@ export class BlitSurfaceCanvas { height: "100%", }); } - if (remotePointerSvg) { + // Guarded like the canvas write above it. This branch is every passive + // view — a dock full of cards — and it runs per presented frame, so five + // unconditional CSSOM setters here were parsing the same strings + // thousands of times a second for a box that never moves. + if (remotePointerSvg && !this._overlayFilled) { + this._overlayFilled = true; Object.assign(remotePointerSvg.style, { position: "absolute", left: "0", @@ -2118,6 +2203,7 @@ export class BlitSurfaceCanvas { } return; } + this._overlayFilled = false; const fw = canvas.width; const fh = canvas.height; if (fw === 0 || fh === 0) return; @@ -2144,6 +2230,8 @@ export class BlitSurfaceCanvas { return; } this._lastLayout = { left, top, w, h }; + // This view's own box moved, which the shared epoch cannot see. + this._imeSyncedEpoch = -1; // All values are integer device pixels converted to CSS pixels, so the // canvas lands on the device grid — a stream served at the size that // was asked for is then blitted 1:1. The container's own ratio, not the @@ -2307,9 +2395,17 @@ export class BlitSurfaceCanvas { } // Flush any pending resize now that we have the surface info. this.flushPendingResize(); - // Repaint on any surface change (e.g. resize, new frame decoded - // while listener was briefly detached). - this.blitFromStore(store); + // Repaint on a change to *this* view's surface (a resize, or its first + // metadata), not on every change the connection publishes. + // + // `onChange` is connection-wide and carries no surface id, and the store + // fires it for a title or app-id change on any surface. Repainting + // unconditionally meant one chatty app renaming its window drove a full + // halving chain plus a layout pass through every mounted view on the page + // — a dock of fifteen cards and three panes is eighteen of them per + // title. The store replaces only the changed surface's object, so + // identity is exactly the "did mine change" test. + if (prev !== this.surface) this.blitFromStore(store); }); // Frame listener — must always be registered so decoded frames are @@ -3096,6 +3192,17 @@ export class BlitSurfaceCanvas { * composition — and everything else goes back to the corner, where a * software keyboard can never cover it. */ + /** + * Park the IME capture element on the caret. + * + * Called from {@link applyLayout}, i.e. once per presented frame, so the + * measuring path is gated on something plausibly having moved since the last + * time it ran: this view's own caret rectangle or box (both of which + * invalidate {@link _imeSyncedEpoch} directly) or the shared + * {@link layoutEpoch}. Guest apps that report a caret at all report it on + * every caret move (GTK/Qt) or throughout a composition (Chromium), so the + * placement stays fresh exactly when the candidate window is on screen. + */ private syncImeTarget(): void { const ta = this.textInput; if (!ta) return; @@ -3105,12 +3212,18 @@ export class BlitSurfaceCanvas { typeof document === "undefined" || document.activeElement !== ta ) { + // Both writes inside placeImeTarget are deduped, so the unfocused case — + // every view but one — costs an identity compare and nothing else. placeImeTarget(ta, null); + this._imeSyncedEpoch = -1; return; } + if (this._imeSyncedEpoch === layoutEpoch) return; + this._imeSyncedEpoch = layoutEpoch; const g = this.drawnGeometry(); if (!g) { placeImeTarget(ta, null); + this._imeSyncedEpoch = -1; return; } // Surface pixels to CSS pixels: the inverse of the pointer path, so the @@ -3131,6 +3244,10 @@ export class BlitSurfaceCanvas { this.textInputCursorRect = state.enabled ? (state.cursorRect ?? null) : null; + // A fresh caret is the main reason to re-place, and the app reports one on + // every caret move — so this, not the frame loop, is what keeps the + // candidate window on the cursor. + this._imeSyncedEpoch = -1; this.syncImeTarget(); if (state.enabled) { @@ -3177,12 +3294,17 @@ export class BlitSurfaceCanvas { ); } + /** `geometry` lets a caller that has already measured this frame pass its + * reading in. `drawnGeometry` calls `getBoundingClientRect`, and the wheel + * path used to take two of those per event: one for its own scaling and one + * in here, either side of a style write. */ private sendPointerAt( clientX: number, clientY: number, type: number, button: number, timeMs = 0, + geometry?: DrawnGeometry | null, ): void { const conn = this.getConn(); if (!conn || !this.canvas || !this.surface || !this._displaySize) return; @@ -3195,7 +3317,7 @@ export class BlitSurfaceCanvas { } else if (type === SURFACE_POINTER_UP) { this.pressedButtons.delete(button); } - const point = this.surfaceWirePoint(clientX, clientY); + const point = this.surfaceWirePoint(clientX, clientY, geometry); if (!point) return; conn.sendSurfacePointer( this._surfaceId, @@ -3221,8 +3343,9 @@ export class BlitSurfaceCanvas { private surfaceWirePoint( clientX: number, clientY: number, + geometry?: DrawnGeometry | null, ): { x: number; y: number } | null { - const point = this.surfacePointFromClient(clientX, clientY); + const point = this.surfacePointFromClient(clientX, clientY, true, geometry); if (!point || !this.surface) return null; return { x: Math.min(Math.max(point.x, 0), Math.max(0, this.surface.width - 1)), @@ -3255,15 +3378,7 @@ export class BlitSurfaceCanvas { * wheel and a drag move content by the same amount on a letterboxed or * downscaled surface. */ - private drawnGeometry(): { - dx: number; - dy: number; - dw: number; - dh: number; - sx: number; - sy: number; - rect: DOMRect; - } | null { + private drawnGeometry(): DrawnGeometry | null { if (!this.canvas || !this.surface) return null; const rect = this.canvas.getBoundingClientRect(); const cw = this.canvas.width; @@ -3300,8 +3415,9 @@ export class BlitSurfaceCanvas { clientX: number, clientY: number, rounded = true, + geometry?: DrawnGeometry | null, ): { x: number; y: number } | null { - const g = this.drawnGeometry(); + const g = geometry ?? this.drawnGeometry(); if (!g) return null; const x = (clientX - g.rect.left - g.dx) * g.sx; const y = (clientY - g.rect.top - g.dy) * g.sy; @@ -3828,7 +3944,10 @@ export class BlitSurfaceCanvas { // under a stationary cursor (including halfway through momentum), which // otherwise leaves no live surface to receive this or any later scroll. // Touch scrolling does the same when the drag first becomes a scroll. - this.sendPointerAt(e.clientX, e.clientY, SURFACE_POINTER_MOVE, 0); + // Reuse the reading taken above rather than measuring again: this runs at + // the trackpad's event rate, and a second getBoundingClientRect here landed + // after applyLayout's style writes had already dirtied layout. + this.sendPointerAt(e.clientX, e.clientY, SURFACE_POINTER_MOVE, 0, 0, g); // The latch has to win before the detent maths below, not just when // labelling the source, or a smooth event ends up carrying notches. diff --git a/js/core/src/BlitTerminalSurface.ts b/js/core/src/BlitTerminalSurface.ts index 5dac7e05..68c887ae 100644 --- a/js/core/src/BlitTerminalSurface.ts +++ b/js/core/src/BlitTerminalSurface.ts @@ -12,7 +12,7 @@ import { assessUrl, openUrlSafely, type UrlAssessment } from "./urlSecurity"; import { devicePixelBox, drawHalved, halve, halvings } from "./downscale"; import { gridCaretRect, placeChip, placeImeTarget } from "./imeTarget"; import { captureDelta } from "./prediction"; -import { WheelDetents } from "./wheel"; +import { WheelDetents, notchedRows } from "./wheel"; /** One screen row's slice of a hyperlink's extent, inclusive of both columns. */ interface LinkSegment { @@ -298,10 +298,19 @@ export class BlitTerminalSurface { /** Inner spacer that gives `scrollEl` enough scrollable content height * for the current scrollback range. */ private scrollSpacer: HTMLDivElement | null = null; - /** True while we're updating `scrollEl.scrollTop` from inside our own - * scrollOffset → scrollTop sync, so the scroll listener doesn't feed - * the change back. */ + /** True while a resize is re-clamping `scrollEl.scrollTop` under us, so the + * scroll listener doesn't read the browser's reflow as the user scrolling. + * A span of time, because a reflow's scroll events cannot be named. */ private suppressScrollSync = false; + /** The exact `scrollTop` the sync last asked for, waiting for its own echo. + * + * Named rather than timed, because a span of time swallows whatever else + * lands inside it. A wheel notch is one scroll event now that its travel + * is quantised — it used to be a burst of six from the browser's scroll + * animation, of which losing one went unnoticed — so a notch that arrived + * during the window lost the whole gesture: the surface moved and nothing + * else did, leaving the reader at the bottom having plainly scrolled up. */ + private pendingScrollTopWrite: number | null = null; /** scrollEl's client height, refreshed from the ResizeObserver and the * scroll listener — both of which run after layout, so the measurement * costs nothing. Never read inside the render loop (see @@ -3111,6 +3120,13 @@ export class BlitTerminalSurface { if (!el) return; this.boundScrollListener = () => { if (this.suppressScrollSync) return; + const pending = this.pendingScrollTopWrite; + if (pending !== null) { + this.pendingScrollTopWrite = null; + // Our own write coming back. Anything else reached the element + // first and is the user's, however close behind the write it was. + if (Math.abs(el.scrollTop - pending) < 0.5) return; + } const t = this.terminal; if (!t) return; const maxLines = t.scrollback_lines(); @@ -3220,38 +3236,45 @@ export class BlitTerminalSurface { this.lastScrollTop = targetTop; return; } - if (drift > 0.5 && !this.gestureOwnsScrollTop(drift, cellH)) { + if (drift > 0.5 && !this.subRowDrift(drift, cellH)) { this.lastScrollTop = targetTop; - this.suppressScrollSync = true; + this.pendingScrollTopWrite = targetTop; el.scrollTop = targetTop; - // The scroll event is async; clear the flag in the next frame. + // A write the browser clamps produces no echo at all, so give the + // claim a frame to live rather than leaving it to match some later + // scroll that happens to land on the same pixel. requestAnimationFrame(() => { - this.suppressScrollSync = false; + this.pendingScrollTopWrite = null; }); } } /** - * True when a scroll gesture is still in flight and the only disagreement - * is where inside a row it stopped. + * True when the only disagreement is where inside a row the surface sits. * * `scrollOffset` is whole lines, so the position it maps back to is the - * nearest row boundary — never more than half a row from wherever the - * user actually is. Writing that back mid-gesture cancels the browser's - * momentum animation and restarts it from a snapped position, once per - * frame, which is what made a flick stutter. The offset the scroll - * listener derived is already correct either way; the write is only a - * cosmetic re-alignment, so it can wait for the gesture to end. + * nearest row boundary — never more than half a row from wherever the user + * actually is. Writing that back is not worth doing at any time, because + * nothing renders from `scrollTop`: the canvas draws rows from + * `scrollOffset`, the scrollbar beside it is ours and drawn from + * `scrollOffset` too, and the surface's own scrollbar is hidden. The + * difference is invisible until the write makes it visible, by taking the + * scroll away from the browser mid-flight and putting it somewhere else. + * + * This used to hold only for the length of a gesture, which cured a flick + * and left the wheel alone: a notch settles in well under + * `SCROLL_SETTLE_MS`, so every one of them ended with up to half a row of + * correction, in whichever direction its remainder fell. It rides the + * render loop, and an idle shell only renders on the cursor blink, so it + * arrived as much as half a second late — long after the wheel had stopped, + * which is what made it read as the terminal moving on its own. * * A jump from somewhere else — Shift+PageUp, a paste, the server - * re-anchoring a scrolled view — moves by rows, not by a fraction of - * one, and still lands immediately. + * re-anchoring a scrolled view — moves by rows, not by a fraction of one, + * and still lands immediately. */ - private gestureOwnsScrollTop(drift: number, cellH: number): boolean { - return ( - drift < cellH && - performance.now() - this.lastUserScrollAt < SCROLL_SETTLE_MS - ); + private subRowDrift(drift: number, cellH: number): boolean { + return drift < cellH; } // --- Mouse input --- @@ -3678,10 +3701,21 @@ export class BlitTerminalSurface { const wheelDetents = new WheelDetents(); const handleCanvasWheel = (e: WheelEvent) => { const t = this.terminal; - if (!t || t.mouse_mode() === 0 || e.shiftKey) return; + if (!t) return; // Ctrl+wheel is how browsers report a pinch-zoom, including macOS // trackpad pinches. It is a zoom request, not a scroll. if (e.ctrlKey) return; + if (t.mouse_mode() === 0 || e.shiftKey) { + // Scrollback navigation. Native scroll does the work; a notched + // wheel only has its travel put back on the row grid first, so the + // sync has no rounding left to write back afterwards. + const rows = notchedRows(e, this.cell.h); + const el = this.scrollEl; + if (rows === 0 || !el) return; + e.preventDefault(); + el.scrollTop += rows * this.cell.h; + return; + } // Claim the gesture even when it hasn't completed a step yet, or the // leftover travel scrolls our own scrollback at the same time. e.preventDefault(); diff --git a/js/core/src/BlitWorkspace.ts b/js/core/src/BlitWorkspace.ts index c48e6950..19484dbb 100644 --- a/js/core/src/BlitWorkspace.ts +++ b/js/core/src/BlitWorkspace.ts @@ -77,9 +77,17 @@ export interface CreateWorkspaceSessionOptions { rows: number; cols: number; tag?: string; + /** Run this through the target server's login shell. */ command?: string; + /** Exec this argv directly, no shell. Needs `FEATURE_CREATE_EXEC`. */ + argv?: readonly string[]; cwdFromSessionId?: SessionId; cwd?: string; + /** Environment overrides for the child. Needs `FEATURE_CREATE_EXEC`. */ + env?: Readonly>; + /** Server-enforced lifetime, armed at creation. Needs + * `FEATURE_PTY_DEADLINE`. */ + deadlineMs?: number; } export interface ResizeWorkspaceSessionOptions { @@ -226,8 +234,11 @@ export class BlitWorkspace { cols: options.cols, tag: options.tag, command: options.command, + argv: options.argv, cwdFromSessionId: options.cwdFromSessionId, cwd: options.cwd, + env: options.env, + deadlineMs: options.deadlineMs, }); return session; } diff --git a/js/core/src/__tests__/BlitConnection.test.ts b/js/core/src/__tests__/BlitConnection.test.ts index f2f759f1..60536dd4 100644 --- a/js/core/src/__tests__/BlitConnection.test.ts +++ b/js/core/src/__tests__/BlitConnection.test.ts @@ -38,6 +38,9 @@ import { CREATE2_WANT_STATUS, FEATURE_CREATE_NONCE, FEATURE_CREATE_STATUS, + FEATURE_CREATE_EXEC, + CREATE2_HAS_ARGV, + CREATE2_HAS_ENV, CLIENT_LIST_WANT_ORIGIN, FEATURE_CLIENT_CONTROL, FEATURE_CLIENT_ORIGIN, @@ -858,6 +861,54 @@ describe("BlitConnection", () => { expect(msg[7] & CREATE2_WANT_STATUS).toBe(CREATE2_WANT_STATUS); }); + it("createSession with argv and env sets the exec features", () => { + transport.pushHello(1, FEATURE_CREATE_NONCE | FEATURE_CREATE_EXEC); + conn.createSession({ + rows: 24, + cols: 80, + argv: ["cargo", "run"], + env: { RUST_LOG: "debug" }, + }); + const msg = transport.sent.find((m) => m[0] === C2S_CREATE2)!; + expect(msg[7] & CREATE2_HAS_ARGV).toBe(CREATE2_HAS_ARGV); + expect(msg[7] & CREATE2_HAS_ENV).toBe(CREATE2_HAS_ENV); + expect(msg[7] & CREATE2_HAS_COMMAND).toBe(0); + }); + + /** An older server does not refuse the unknown flag — it ignores it and + * starts a plain shell, or reads the env block as command text. So the + * check has to happen before anything goes on the wire. */ + it("createSession refuses argv or env the server cannot honour", async () => { + transport.pushHello(1, FEATURE_CREATE_NONCE); + await expect( + conn.createSession({ rows: 24, cols: 80, argv: ["htop"] }), + ).rejects.toThrow(/argv or environment/); + await expect( + conn.createSession({ rows: 24, cols: 80, env: { A: "1" } }), + ).rejects.toThrow(/argv or environment/); + expect(transport.sent.find((m) => m[0] === C2S_CREATE2)).toBeUndefined(); + }); + + it("createSession refuses a deadline the server cannot honour", async () => { + transport.pushHello(1, FEATURE_CREATE_NONCE); + await expect( + conn.createSession({ rows: 24, cols: 80, deadlineMs: 1000 }), + ).rejects.toThrow(/deadlines/); + expect(transport.sent.find((m) => m[0] === C2S_CREATE2)).toBeUndefined(); + }); + + it("createSession reports an argv session's command", async () => { + transport.pushHello(1, FEATURE_CREATE_NONCE | FEATURE_CREATE_EXEC); + const promise = conn.createSession({ + rows: 24, + cols: 80, + argv: ["cargo", "test"], + }); + const msg = transport.sent.find((m) => m[0] === C2S_CREATE2)!; + transport.pushCreatedN(msg[1] | (msg[2] << 8), 9, ""); + expect((await promise).command).toBe("cargo test"); + }); + it("S2C_CREATE_FAILED rejects only the matching nonce", async () => { transport.pushHello(1, FEATURE_CREATE_NONCE | FEATURE_CREATE_STATUS); const first = conn.createSession({ rows: 24, cols: 80, tag: "a" }); @@ -2252,6 +2303,60 @@ describe("BlitConnection surface subscriptions", () => { expect(lastMaxFps()).toBe(15); }); + it("mints view tokens no other connection can collide with", () => { + const other = new BlitConnection({ + id: "other", + transport: new MockTransport(), + wasm, + autoConnect: false, + }); + try { + // Same ordinal on both, because each counter starts at zero. + expect(other.allocSurfaceViewId()).not.toBe(conn.allocSurfaceViewId()); + } finally { + other.dispose(); + } + }); + + it("keeps a pane's request when a view that arrived from another connection shares the surface", () => { + // A canvas mints its token once and keeps it across setConnectionId, so a + // dock card re-pointed from another server registers here under a foreign + // token. It must not land on the same `views` entry as this connection's + // own pane. + const foreign = new BlitConnection({ + id: "other", + transport: new MockTransport(), + wasm, + autoConnect: false, + }); + const card = foreign.allocSurfaceViewId(); + foreign.dispose(); + const pane = conn.allocSurfaceViewId(); + + conn.sendSurfaceSubscribe(1, pane, null, 0); + conn.sendSurfaceSubscribe(1, card, { width: 512, height: 256 }, 15); + expect(lastMaxFps()).toBe(0); + expect(lastTarget()).toBeNull(); + + // The card's box crosses an octave — any surface resize does this, because + // the card's height is derived from the surface's aspect. It re-derives + // its own request and must not speak for the pane. + conn.setSurfaceViewTarget(1, card, { width: 512, height: 256 }, 15); + expect(lastMaxFps()).toBe(0); + expect(lastTarget()).toBeNull(); + + // The card scrolls out of the dock. The pane keeps the stream. + const before = transport.sent.length; + conn.sendSurfaceUnsubscribe(1, card); + expect(lastMaxFps()).toBe(0); + expect(lastTarget()).toBeNull(); + expect( + transport.sent + .slice(before) + .some((m) => m[0] === C2S_SURFACE_UNSUBSCRIBE), + ).toBe(false); + }); + it("applies and removes a global frame-rate cap", () => { conn.sendSurfaceSubscribe(1, conn.allocSurfaceViewId(), null, 0); expect(lastMaxFps()).toBe(0); diff --git a/js/core/src/__tests__/BlitSurfaceCanvas.test.ts b/js/core/src/__tests__/BlitSurfaceCanvas.test.ts index b334d7f9..a748925e 100644 --- a/js/core/src/__tests__/BlitSurfaceCanvas.test.ts +++ b/js/core/src/__tests__/BlitSurfaceCanvas.test.ts @@ -3205,6 +3205,7 @@ function attachTyping() { pointers.push({ type, button }); }, sendSurfaceFocus: () => {}, + sendSurfaceAxis2: () => {}, noteBrowserClipboardMayHaveChanged: () => {}, surfaceStore: new Proxy( { @@ -4304,3 +4305,234 @@ describe("BlitSurfaceCanvas macOS dead keys", () => { surface.dispose(); }); }); + +/** These pin the per-frame cost of the present path. `applyLayout` runs on + * every presented frame, so anything that measures or writes layout in there + * is paid at the stream's frame rate — and those writes then invalidate layout + * for the input handlers' own reads, which is what made scrolling a focused + * pane expensive. */ +describe("BlitSurfaceCanvas per-frame layout cost", () => { + /** Count forced layout reads on the canvas. */ + function countRects(canvas: HTMLCanvasElement): () => number { + let n = 0; + canvas.getBoundingClientRect = () => { + n++; + return { + width: 800, + height: 600, + left: 0, + top: 0, + right: 800, + bottom: 600, + } as DOMRect; + }; + return () => n; + } + + /** The IME path only engages for a *focused* capture element, and jsdom only + * focuses an element that is in the document. */ + function focusedWithCaret(x = 10) { + const harness = attachTyping(); + const container = harness.canvas.parentElement; + if (!container) throw new Error("Expected a container"); + document.body.appendChild(container); + harness.ta.focus(); + if (document.activeElement !== harness.ta) { + throw new Error("Expected the capture element to hold focus"); + } + harness.requestTextInput({ + enabled: true, + requested: false, + hint: 0, + purpose: 0, + cursorRect: { x, y: 20, width: 1, height: 16 }, + }); + return { ...harness, container }; + } + + /** Stand in for the presenter: applyLayout is what each presented frame + * reaches, via blitFromStore. */ + function presentFrames(surface: BlitSurfaceCanvas, n: number): void { + for (let i = 0; i < n; i++) surface.setDisplaySize(800, 600, 120); + } + + afterEach(() => { + document.body.innerHTML = ""; + }); + + it("does not measure per frame while the IME target is parked", () => { + const { surface, canvas, container } = focusedWithCaret(); + const rects = countRects(canvas); + // One warm-up: the caret above invalidated the placement, so the first + // frame after it legitimately measures. + presentFrames(surface, 1); + const settled = rects(); + expect(settled).toBeGreaterThan(0); + + presentFrames(surface, 30); + // Nothing moved, so thirty more frames cost no layout at all. + expect(rects()).toBe(settled); + surface.dispose(); + container.remove(); + }); + + it("re-places the IME target when the app reports a new caret", () => { + const { surface, canvas, container, requestTextInput } = focusedWithCaret(); + presentFrames(surface, 1); + const rects = countRects(canvas); + + // A caret move is the main reason to re-place, and GTK/Qt send one on every + // cursor move, so this — not the frame loop — is what keeps the candidate + // window on the cursor. + requestTextInput({ + enabled: true, + requested: false, + hint: 0, + purpose: 0, + cursorRect: { x: 40, y: 20, width: 1, height: 16 }, + }); + expect(rects()).toBeGreaterThan(0); + surface.dispose(); + container.remove(); + }); + + it("re-places the IME target after something scrolls the pane", () => { + const { surface, canvas, container } = focusedWithCaret(); + presentFrames(surface, 2); + const rects = countRects(canvas); + presentFrames(surface, 1); + const settled = rects(); + + // A scroll in any ancestor moves the pane on screen with no notification of + // its own, which is why the frame loop used to measure unconditionally. + window.dispatchEvent(new Event("scroll")); + presentFrames(surface, 1); + expect(rects()).toBeGreaterThan(settled); + surface.dispose(); + container.remove(); + }); + + it("measures once per wheel event, not twice", () => { + const { surface, canvas } = attachTyping(); + const rects = countRects(canvas); + canvas.dispatchEvent( + new WheelEvent("wheel", { + deltaY: 12, + clientX: 100, + clientY: 100, + bubbles: true, + cancelable: true, + }), + ); + // One reading, reused for both the axis scaling and the pointer re-seed that + // precedes it on the wire. + expect(rects()).toBe(1); + surface.dispose(); + }); +}); + +describe("BlitSurfaceCanvas change fan-out", () => { + /** `SurfaceStore.onChange` is connection-wide and carries no surface id, and + * the store fires it for a title or app-id change on *any* surface. Every + * mounted view listens, so repainting unconditionally made one chatty app + * renaming its window drive a full halving chain plus a layout pass through + * every card and pane on the page. */ + function mountWithStore() { + let info: BlitSurface | undefined = { + width: 1920, + height: 1080, + } as BlitSurface; + let change: (() => void) | undefined; + let canvasReads = 0; + const store = { + getSurface: () => info, + getCanvas: () => { + canvasReads++; + return null; + }, + getCursor: () => "default", + canDecodeVideo: false, + generation: 0, + onChange: (cb: () => void) => { + change = cb; + return () => {}; + }, + onCursor: () => () => {}, + onFrame: () => () => {}, + }; + const workspace = { + getConnection: () => ({ + surfaceStore: store, + allocSurfaceViewId: () => "c1:s1", + sendSurfaceSubscribe: () => {}, + sendSurfaceUnsubscribe: () => {}, + }), + subscribe: () => () => {}, + } as unknown as BlitWorkspace; + const surface = new BlitSurfaceCanvas({ + workspace, + connectionId: "conn-1" as never, + surfaceId: 7, + resizable: true, + }); + surface.attach(document.createElement("div")); + return { + surface, + reads: () => canvasReads, + fireChange: () => change?.(), + /** Stand in for the store replacing this surface's object, which is what + * it does for a resize. */ + replaceSurface: () => { + info = { ...(info as BlitSurface) }; + }, + }; + } + + it("ignores a change that did not touch this view's surface", () => { + const { surface, reads, fireChange } = mountWithStore(); + const before = reads(); + for (let i = 0; i < 10; i++) fireChange(); + // Another surface's title moved; nothing here needs redrawing. + expect(reads()).toBe(before); + surface.dispose(); + }); + + it("still repaints when this view's own surface changes", () => { + const { surface, reads, fireChange, replaceSurface } = mountWithStore(); + const before = reads(); + replaceSurface(); + fireChange(); + expect(reads()).toBeGreaterThan(before); + surface.dispose(); + }); +}); + +describe("BlitSurfaceCanvas passive layout", () => { + /** A card in the dock sizes itself from the surface's aspect with + * `height: auto`, so its canvas has to stay *in flow* and fill the box. A + * view that reports a display size gets absolutely positioned instead, which + * leaves `height: auto` with nothing to measure — the sidebar thumbnails + * collapsed exactly this way when `resizable` started defaulting to true. */ + it("leaves a view with no display size filling its box, in flow", () => { + const { surface, canvas } = attachCanvas(); + expect(canvas.style.width).toBe("100%"); + expect(canvas.style.height).toBe("100%"); + expect(canvas.style.position).toBe(""); + surface.dispose(); + }); + + it("positions a sized view absolutely, and puts it back on the way out", () => { + const { surface, canvas } = attachCanvas(); + surface.setDisplaySize(800, 600, 120); + expect(canvas.style.position).toBe("absolute"); + expect(canvas.style.width).toBe("800px"); + + // Going back to a passive preview has to restore the fill, or a card that + // was once a pane keeps a stale pixel height. + surface.setDisplaySize(null); + expect(canvas.style.position).toBe(""); + expect(canvas.style.width).toBe("100%"); + expect(canvas.style.height).toBe("100%"); + surface.dispose(); + }); +}); diff --git a/js/core/src/__tests__/BlitTerminalSurface.test.ts b/js/core/src/__tests__/BlitTerminalSurface.test.ts index d492b2c9..f51706af 100644 --- a/js/core/src/__tests__/BlitTerminalSurface.test.ts +++ b/js/core/src/__tests__/BlitTerminalSurface.test.ts @@ -1631,6 +1631,370 @@ describe("BlitTerminalSurface native scroll surface", () => { }); }); +describe("BlitTerminalSurface scrollback against a server that answers", () => { + // One gesture is many scroll events — a wheel notch Chromium animates over + // several frames, a momentum flick on an iPad, dozens. Each is reported as + // a relative move, and each report the server answers comes back absolute + // and a round trip late. Adopting a late answer drags the view back to + // where the gesture used to be, and the next delta — measured from there — + // comes out too big, so the view lurches past where the finger asked. + beforeEach(() => { + mockCanvasContext(); + vi.stubGlobal( + "requestAnimationFrame", + vi.fn((cb: FrameRequestCallback) => { + cb(0); + return 1; + }), + ); + }); + afterEach(() => { + vi.unstubAllGlobals(); + vi.restoreAllMocks(); + }); + + const LINES = 1000; + const CELL_H = 10; + /** jsdom reports no scrollHeight, so the code falls back to the model. */ + const MAX_TOP = LINES * CELL_H; + + /** + * A surface wired to a server that holds its own offset, applies each + * relative move to it, and answers `answerEverything` moves later. + */ + function rig(lagFrames: number, answerEverything: boolean) { + const s = new BlitTerminalSurface({ sessionId: null }); + const el = document.createElement("div"); + const spacer = document.createElement("div"); + el.appendChild(spacer); + Object.defineProperty(el, "clientHeight", { + configurable: true, + value: 80, + }); + + // @ts-expect-error — install DOM/terminal stubs for the private sync. + s.scrollEl = el; + // @ts-expect-error — install DOM/terminal stubs for the private sync. + s.scrollSpacer = spacer; + // @ts-expect-error — only scrollback_lines is read here. + s.terminal = { scrollback_lines: () => LINES }; + // @ts-expect-error — only cell.h is read by the scroll surface methods. + s.cell = { h: CELL_H }; + + let anchor: ((offset: number) => void) | null = null; + // @ts-expect-error — minimal connection: status gate plus the anchor hook. + s["_blitConn"] = { + transport: { status: "connected" }, + addScrollAnchorListener: (_id: string, cb: (o: number) => void) => { + anchor = cb; + return () => {}; + }, + }; + // @ts-expect-error — the listener is per-session. + s["_sessionId"] = "s1"; + + let serverOffset = 0; + let frame = 0; + const inFlight: { at: number; offset: number }[] = []; + const sent: number[] = []; + + // @ts-expect-error — minimal workspace: only the scroll verbs are used. + s["_workspace"] = { + scrollSessionBy: (_id: string, _abs: number, lines: number) => { + sent.push(lines); + const requested = serverOffset + lines; + serverOffset = Math.max(0, Math.min(LINES, requested)); + if (answerEverything || requested !== serverOffset) { + inFlight.push({ at: frame + lagFrames, offset: serverOffset }); + } + }, + scrollSession: () => {}, + }; + + // @ts-expect-error — wire the private listeners. + s["setupScrollAnchorListener"](); + // @ts-expect-error — wire the private listeners. + s["setupScrollSurface"](); + + /** One frame: the browser moves scrollTop, then any answer that has + * finished its round trip lands. */ + const step = (scrollTop: number) => { + el.scrollTop = scrollTop; + // @ts-expect-error — the rAF stub already cleared the listener handle. + s.boundScrollListener(); + frame++; + while (inFlight.length && inFlight[0].at <= frame) { + anchor!(inFlight.shift()!.offset); + } + }; + + /** Drain the wire once the gesture has stopped. */ + const settle = () => { + while (inFlight.length) { + frame++; + anchor!(inFlight.shift()!.offset); + } + }; + + return { step, settle, sent, server: () => serverOffset }; + } + + /** Twelve rows of travel, two rows a frame, the way one notch arrives. */ + const oneNotch = (step: (top: number) => void) => { + for (let i = 1; i <= 6; i++) step(MAX_TOP - i * 20); + }; + + it("lands a notch where it pointed when nothing answers back", () => { + const { step, settle, sent, server } = rig(2, false); + oneNotch(step); + settle(); + expect(sent).toEqual([2, 2, 2, 2, 2, 2]); + expect(server()).toBe(12); + }); + + it("would overshoot a notch if every move were answered", () => { + // The behaviour this exists to prevent, kept as the thing being ruled + // out: the doubled deltas are the answers landing mid-gesture. + const { step, settle, sent, server } = rig(2, true); + oneNotch(step); + settle(); + expect(sent).toEqual([2, 2, 4, 4, 2]); + expect(server()).toBe(14); + }); + + it("lands a flick where it pointed, however long the wire is", () => { + for (const lag of [1, 2, 5]) { + const { step, settle, server } = rig(lag, false); + for (let i = 1; i <= 18; i++) step(MAX_TOP - i * 20); + settle(); + expect({ lag, offset: server() }).toEqual({ lag, offset: 36 }); + } + }); +}); + +describe("BlitTerminalSurface wheel over the scrollback", () => { + let now = 0; + beforeEach(() => { + now = 1000; + vi.spyOn(performance, "now").mockImplementation(() => now); + mockCanvasContext(); + vi.stubGlobal( + "requestAnimationFrame", + vi.fn((cb: FrameRequestCallback) => { + cb(0); + return 1; + }), + ); + vi.stubGlobal("cancelAnimationFrame", vi.fn()); + vi.stubGlobal( + "ResizeObserver", + class { + observe() {} + disconnect() {} + }, + ); + }); + afterEach(() => { + vi.unstubAllGlobals(); + vi.restoreAllMocks(); + }); + + const LINES = 1000; + const CELL_H = 19; // 120px is 6.3 of these — the awkward case + + /** A surface at a plain prompt, where the wheel navigates scrollback. */ + function attachScrollback() { + const surface = new BlitTerminalSurface({ sessionId: "s1" }); + surface.attach(document.createElement("div")); + // @ts-expect-error — minimal connection exposing a connected transport. + surface["_blitConn"] = { transport: { status: "connected" } }; + // @ts-expect-error — no app is reading the mouse. + surface["terminal"] = { + mouse_mode: () => 0, + scrollback_lines: () => LINES, + }; + // @ts-expect-error — a known cell for the row maths. + surface["cell"] = { h: CELL_H, w: 8, pw: 8, ph: CELL_H }; + const el = surface["scrollEl"]; + if (!el) throw new Error("expected a scroll surface"); + el.scrollTop = LINES * CELL_H; // parked at the live bottom + + const notch = (deltaY: number, deltaMode = 0) => { + const e = new WheelEvent("wheel", { cancelable: true }); + Object.defineProperties(e, { + deltaY: { value: deltaY }, + deltaX: { value: 0 }, + deltaMode: { value: deltaMode }, + }); + el.dispatchEvent(e); + // jsdom does not fire `scroll` for a programmatic scrollTop. + // @ts-expect-error — the listener the real event would have run. + surface["boundScrollListener"]?.(); + return e; + }; + /** The wheel rests, and the next render syncs — a cursor blink will do. */ + const settle = () => { + now += 200; + const before = el.scrollTop; + // @ts-expect-error — the render loop's idempotent sync. + surface["syncScrollSurface"](true); + return el.scrollTop - before; + }; + // @ts-expect-error — read the offset the listener derived. + const offset = () => surface["scrollOffset"] as number; + return { surface, el, notch, settle, offset }; + } + + it("moves every notch the same whole number of rows", () => { + const { notch, settle, offset } = attachScrollback(); + const steps: number[] = []; + let prev = 0; + for (let n = 0; n < 12; n++) { + notch(-120); + steps.push(offset() - prev); + prev = offset(); + settle(); + } + // 120px over a 19px cell: six rows, twelve times, not 6/7 alternating. + expect(steps).toEqual(Array(12).fill(6)); + }); + + it("leaves the surface nothing to snap back once the notch settles", () => { + // The jank: the sync used to write the rounding back up to half a row + // later, in whichever direction the remainder fell, as late as the next + // cursor blink — +6, -7, -1, +5, -8 … px at this cell height. + const { notch, settle, el } = attachScrollback(); + const start = el.scrollTop; + const snaps: number[] = []; + for (let n = 0; n < 12; n++) { + notch(-120); + snaps.push(settle()); + } + expect(snaps).toEqual(Array(12).fill(0)); + // jsdom does no scrolling of its own, so assert the notches actually + // moved the surface — otherwise "nothing snapped back" is vacuous. + expect(el.scrollTop).toBe(start - 12 * 6 * CELL_H); + }); + + it("leaves a surface parked between rows exactly where it is", () => { + // Nothing renders from scrollTop — the canvas draws rows from the + // offset, our scrollbar likewise, and the surface's own is hidden — so + // a position inside a row is invisible until squaring it up makes it + // visible. A trackpad lands here on every gesture. + const { el, settle, surface } = attachScrollback(); + el.scrollTop = LINES * CELL_H - 100; // 5.26 rows: not on the grid + // @ts-expect-error — the listener the real scroll event would have run. + surface["boundScrollListener"](); + expect(settle()).toBe(0); + }); + + it("still lands a jump that moved by whole rows", () => { + // Shift+PageUp, a paste, the server re-anchoring: these move the offset + // without touching the surface, and the surface has to follow. + const { el, settle, surface } = attachScrollback(); + // @ts-expect-error — the listener the real scroll event would have run. + surface["boundScrollListener"](); + // @ts-expect-error — what the scrollback-navigation keys do. + surface["scrollOffset"] = 3; + expect(settle()).toBe(-3 * CELL_H); + expect(el.scrollTop).toBe((LINES - 3) * CELL_H); + }); + + it("claims the notch so the browser does not scroll it as well", () => { + const { notch } = attachScrollback(); + expect(notch(-120).defaultPrevented).toBe(true); + }); + + it("leaves a trackpad to the browser's own scrolling", () => { + const { notch, el } = attachScrollback(); + const before = el.scrollTop; + const e = notch(-53.5); + expect(e.defaultPrevented).toBe(false); + expect(el.scrollTop).toBe(before); + }); + + /** Hold rAF callbacks instead of running them, so the frame the sync uses + * as a backstop stays open for the length of the test. */ + function deferFrames() { + const queued: FrameRequestCallback[] = []; + vi.stubGlobal( + "requestAnimationFrame", + vi.fn((cb: FrameRequestCallback) => { + queued.push(cb); + return 1; + }), + ); + return () => { + const run = queued.splice(0); + for (const cb of run) cb(0); + }; + } + + it("keeps a notch that lands while the sync's own write is in flight", () => { + // The sync claims the echo of the scrollTop it wrote. It used to claim a + // frame instead, and once a notch became a single scroll event rather + // than an animated burst, a notch inside that frame was the whole + // gesture — the surface moved and nothing else did, so the reader stayed + // at the bottom having plainly scrolled up. + const { surface, notch, offset } = attachScrollback(); + // @ts-expect-error — the listener the real scroll event would have run. + surface["boundScrollListener"](); + // A whole-row jump from elsewhere, which the sync does still write. + // @ts-expect-error — what the scrollback-navigation keys do. + surface["scrollOffset"] = 3; + now += 200; + const runFrames = deferFrames(); + // @ts-expect-error — the write that claims its own echo. + surface["syncScrollSurface"](true); + // @ts-expect-error — the claim is outstanding: no echo has arrived yet. + expect(surface["pendingScrollTopWrite"]).not.toBeNull(); + + const before = offset(); + notch(-120); // the user's wheel beats the echo to the element + expect(offset()).toBe(before + 6); + runFrames(); + }); + + it("still ignores the echo of the sync's own write", () => { + const { surface, offset } = attachScrollback(); + // @ts-expect-error — the listener the real scroll event would have run. + surface["boundScrollListener"](); + // A whole-row jump from elsewhere, which the sync does still write. + // @ts-expect-error — what the scrollback-navigation keys do. + surface["scrollOffset"] = 3; + now += 200; + const runFrames = deferFrames(); + // @ts-expect-error — the write that claims its own echo. + surface["syncScrollSurface"](true); + const settled = offset(); + // Something else moved the offset, so processing the echo would show. + // @ts-expect-error — a re-anchor arriving between the write and its echo. + surface["scrollOffset"] = settled + 3; + + // The browser now reports the position the sync itself asked for. + // @ts-expect-error — the echo, which must change nothing. + surface["boundScrollListener"](); + expect(offset()).toBe(settled + 3); + // @ts-expect-error — and the claim is spent, not left to match again. + expect(surface["pendingScrollTopWrite"]).toBeNull(); + runFrames(); + }); + + it("still lets ctrl+wheel through as a zoom", () => { + const { surface, el } = attachScrollback(); + const before = el.scrollTop; + const e = new WheelEvent("wheel", { cancelable: true, ctrlKey: true }); + Object.defineProperties(e, { + deltaY: { value: -120 }, + deltaMode: { value: 0 }, + }); + el.dispatchEvent(e); + expect(e.defaultPrevented).toBe(false); + expect(el.scrollTop).toBe(before); + expect(surface).toBeTruthy(); + }); +}); + describe("BlitTerminalSurface wheel in mouse-reporting apps", () => { beforeEach(() => { mockCanvasContext(); diff --git a/js/core/src/__tests__/bsp-tile.test.ts b/js/core/src/__tests__/bsp-tile.test.ts index c5515cfe..5d694eec 100644 --- a/js/core/src/__tests__/bsp-tile.test.ts +++ b/js/core/src/__tests__/bsp-tile.test.ts @@ -2,7 +2,9 @@ import { describe, it, expect } from "vitest"; import { editorAssignment, diffAssignment, + manageAssignment, parseDiffArg, + isContentAssignment, isTileAssignment, parseTileAssignment, } from "../bsp/layout"; @@ -43,6 +45,24 @@ describe("tile assignments (docs/ide-plan.md PR-6)", () => { } }); + // A manage tile's address is its connection and nothing else. The trailing + // colon is load-bearing: parseTileAssignment splits on the first ":" after + // the prefix, so "manage:hound" (no colon left to split on) parses as + // nothing at all — and a tile that fails to parse renders an empty pane. + it("round-trips a manage assignment, arg and all", () => { + const m = manageAssignment("hound"); + expect(isTileAssignment(m)).toBe(true); + expect(isContentAssignment(m)).toBe(true); + expect(parseTileAssignment(m)).toEqual({ + kind: "manage", + connectionId: "hound", + arg: "", + }); + // What the tab registry does to it and back (stripConn/withConn). + expect(parseTileAssignment(`manage:hound:`)).not.toBeNull(); + expect(parseTileAssignment("manage:hound")).toBeNull(); + }); + it("does not treat sessions or surfaces as tiles", () => { expect(isTileAssignment("surface:local:3")).toBe(false); expect(isTileAssignment("local:5")).toBe(false); diff --git a/js/core/src/__tests__/protocol.test.ts b/js/core/src/__tests__/protocol.test.ts index 8307b5fa..c9062c01 100644 --- a/js/core/src/__tests__/protocol.test.ts +++ b/js/core/src/__tests__/protocol.test.ts @@ -49,6 +49,9 @@ import { CREATE2_HAS_SRC_PTY, CREATE2_HAS_COMMAND, CREATE2_HAS_CWD, + CREATE2_HAS_DEADLINE, + CREATE2_HAS_ENV, + CREATE2_HAS_ARGV, C2S_SURFACE_POINTER_AXIS2, C2S_SURFACE_ACK, C2S_SURFACE_SUBSCRIBE, @@ -360,6 +363,129 @@ describe("protocol message builders", () => { CREATE2_HAS_CWD | CREATE2_HAS_COMMAND | CREATE2_WANT_STATUS, ); }); + + it("with a deadline, before any command bytes", () => { + const msg = buildCreate2Message(0, 24, 80, { + cwd: "/tmp", + deadlineMs: 5_000, + command: "sleep 60", + }); + expect(msg[7]).toBe( + CREATE2_HAS_CWD | CREATE2_HAS_DEADLINE | CREATE2_HAS_COMMAND, + ); + const cwdLen = msg[10] | (msg[11] << 8); + expect(textDecoder.decode(msg.subarray(12, 12 + cwdLen))).toBe("/tmp"); + let cursor = 12 + cwdLen; + const ms = + msg[cursor] | + (msg[cursor + 1] << 8) | + (msg[cursor + 2] << 16) | + (msg[cursor + 3] << 24); + expect(ms).toBe(5_000); + cursor += 4; + expect(textDecoder.decode(msg.subarray(cursor))).toBe("sleep 60"); + }); + + it("with argv instead of a command", () => { + const msg = buildCreate2Message(0, 24, 80, { + argv: ["cargo", "test", "--release"], + }); + expect(msg[7]).toBe(CREATE2_HAS_ARGV); + let cursor = 10; + expect(msg[cursor] | (msg[cursor + 1] << 8)).toBe(3); + cursor += 2; + const args: string[] = []; + for (let i = 0; i < 3; i++) { + const len = + msg[cursor] | + (msg[cursor + 1] << 8) | + (msg[cursor + 2] << 16) | + (msg[cursor + 3] << 24); + cursor += 4; + args.push(textDecoder.decode(msg.subarray(cursor, cursor + len))); + cursor += len; + } + expect(args).toEqual(["cargo", "test", "--release"]); + expect(msg.length).toBe(cursor); + }); + + it("with an environment, before the argv", () => { + const msg = buildCreate2Message(0, 24, 80, { + env: { RUST_LOG: "debug", EMPTY: "" }, + argv: ["cargo", "run"], + }); + expect(msg[7]).toBe(CREATE2_HAS_ENV | CREATE2_HAS_ARGV); + let cursor = 10; + expect(msg[cursor] | (msg[cursor + 1] << 8)).toBe(2); + cursor += 2; + const env: [string, string][] = []; + for (let i = 0; i < 2; i++) { + const keyLen = msg[cursor] | (msg[cursor + 1] << 8); + cursor += 2; + const key = textDecoder.decode(msg.subarray(cursor, cursor + keyLen)); + cursor += keyLen; + const valueLen = + msg[cursor] | + (msg[cursor + 1] << 8) | + (msg[cursor + 2] << 16) | + (msg[cursor + 3] << 24); + cursor += 4; + env.push([ + key, + textDecoder.decode(msg.subarray(cursor, cursor + valueLen)), + ]); + cursor += valueLen; + } + expect(env).toEqual([ + ["RUST_LOG", "debug"], + ["EMPTY", ""], + ]); + // The argv block follows, so the env block's own length was honoured. + expect(msg[cursor] | (msg[cursor + 1] << 8)).toBe(2); + }); + + it("refuses what the server would refuse", () => { + expect(() => + buildCreate2Message(0, 24, 80, { argv: ["sh"], command: "sh" }), + ).toThrow(/exclusive/); + expect(() => buildCreate2Message(0, 24, 80, { argv: [] })).toThrow( + /empty/, + ); + expect(() => + buildCreate2Message(0, 24, 80, { env: { "A=B": "1" } }), + ).toThrow(/environment key/); + expect(() => + buildCreate2Message(0, 24, 80, { env: { A: "x\0y" } }), + ).toThrow(/NUL/); + expect(() => + buildCreate2Message(0, 24, 80, { + env: [ + ["A", "1"], + ["A", "2"], + ], + }), + ).toThrow(/duplicate/); + }); + + /** An empty argument is exactly what the legacy NUL spelling could not + * carry, so it is the sharpest check that argv is length-prefixed. */ + it("keeps an empty argument", () => { + const msg = buildCreate2Message(0, 24, 80, { argv: ["sh", "-c", ""] }); + let cursor = 10; + expect(msg[cursor] | (msg[cursor + 1] << 8)).toBe(3); + cursor += 2; + const lengths: number[] = []; + for (let i = 0; i < 3; i++) { + const len = + msg[cursor] | + (msg[cursor + 1] << 8) | + (msg[cursor + 2] << 16) | + (msg[cursor + 3] << 24); + lengths.push(len); + cursor += 4 + len; + } + expect(lengths).toEqual([2, 2, 0]); + }); }); }); diff --git a/js/core/src/__tests__/surfaceResize.test.ts b/js/core/src/__tests__/surfaceResize.test.ts new file mode 100644 index 00000000..74cdd0d4 --- /dev/null +++ b/js/core/src/__tests__/surfaceResize.test.ts @@ -0,0 +1,205 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; +import { clampZoom, driveSurfaceResize } from "../surfaceResize"; +import type { SurfaceResizeTarget, SurfaceZoom } from "../surfaceResize"; + +/** Records what a `BlitSurfaceCanvas` would have been told. */ +function fakeTarget() { + const displaySizes: (number | null)[][] = []; + const resizes: number[][] = []; + const target: SurfaceResizeTarget = { + setDisplaySize(width, height, scale120, cssScale120) { + displaySizes.push([ + width, + height ?? null, + scale120 ?? null, + cssScale120 ?? null, + ]); + }, + requestResize(width, height, scale120) { + resizes.push([width, height, scale120]); + }, + }; + return { target, displaySizes, resizes }; +} + +function container(width: number, height: number): HTMLElement { + const el = document.createElement("div"); + el.getBoundingClientRect = () => + ({ + width, + height, + left: 0, + top: 0, + right: width, + bottom: height, + }) as DOMRect; + return el; +} + +describe("clampZoom", () => { + it("falls back to 1 for anything that is not a usable factor", () => { + for (const bad of [undefined, NaN, Infinity, 0, -2]) { + expect(clampZoom(bad as number | undefined)).toBe(1); + } + }); + + it("clamps to a range both ends of the stack can lay out", () => { + expect(clampZoom(0.1)).toBe(0.25); + expect(clampZoom(9)).toBe(4); + expect(clampZoom(1.25)).toBe(1.25); + }); +}); + +describe("driveSurfaceResize", () => { + let callbacks: ResizeObserverCallback[] = []; + const disconnect = vi.fn(); + + /** Deliver a box to every live observer, in CSS pixels only — the shape a + * browser without `devicePixelContentBoxSize` reports. */ + function resizeTo(width: number, height: number): void { + const entry = { contentRect: { width, height } } as ResizeObserverEntry; + for (const cb of callbacks) cb([entry], null as never); + } + + beforeEach(() => { + callbacks = []; + disconnect.mockClear(); + vi.stubGlobal("devicePixelRatio", 1); + vi.stubGlobal( + "ResizeObserver", + class { + constructor(cb: ResizeObserverCallback) { + callbacks.push(cb); + } + observe = vi.fn(); + disconnect = disconnect; + }, + ); + vi.useFakeTimers(); + }); + + afterEach(() => { + vi.useRealTimers(); + vi.unstubAllGlobals(); + }); + + it("reports the container's box as a display size on the way up", () => { + const { target, displaySizes } = fakeTarget(); + driveSurfaceResize(target, container(800, 600)); + // A view that never reports one takes no input at all and is served a + // thumbnail-grade stream, so this is the whole point of the driver. + expect(displaySizes).toEqual([[800, 600, 120, 120]]); + }); + + it("rounds each extent down to even", () => { + const { target, displaySizes } = fakeTarget(); + // The encoder rounds down to even on its own; asking for an odd extent + // returns a frame a pixel short on that axis and letterboxes the rest. + driveSurfaceResize(target, container(801, 603)); + expect(displaySizes[0]?.slice(0, 2)).toEqual([800, 602]); + }); + + it("scales by devicePixelRatio and reports the ratio it measured", () => { + vi.stubGlobal("devicePixelRatio", 2); + const { target, displaySizes } = fakeTarget(); + driveSurfaceResize(target, container(400, 300)); + expect(displaySizes).toEqual([[800, 600, 240, 240]]); + }); + + it("sends the first size at wire speed and settles on the last", () => { + const { target, resizes } = fakeTarget(); + driveSurfaceResize(target, container(800, 600)); + // Leading edge: a new interaction must not wait out the debounce. + expect(resizes).toEqual([[800, 600, 120]]); + + resizeTo(820, 600); + resizeTo(840, 600); + // Mid-drag sizes are held back... + expect(resizes).toHaveLength(1); + vi.advanceTimersByTime(30); + // ...and only the last one lands. + expect(resizes).toEqual([ + [800, 600, 120], + [840, 600, 120], + ]); + }); + + it("does not re-ask for a size the server already has", () => { + const { target, resizes } = fakeTarget(); + driveSurfaceResize(target, container(800, 600)); + resizes.length = 0; + resizeTo(800, 600); + vi.advanceTimersByTime(30); + expect(resizes).toEqual([]); + }); + + it("treats a gap since the last box as the start of a fresh drag", () => { + const { target, resizes } = fakeTarget(); + driveSurfaceResize(target, container(800, 600)); + vi.advanceTimersByTime(300); + resizes.length = 0; + resizeTo(900, 600); + // No debounce wait: each user-visible drag gets a leading-edge dispatch. + expect(resizes).toEqual([[900, 600, 120]]); + }); + + it("multiplies the display's DPI in relative mode", () => { + const { target, displaySizes } = fakeTarget(); + let zoom: SurfaceZoom = { zoom: 1.5, mode: "relative" }; + driveSurfaceResize(target, container(800, 600), () => zoom); + // The pane still holds 800x600 device pixels; only the scale moves. + expect(displaySizes).toEqual([[800, 600, 180, 120]]); + }); + + it("names the surface scale directly in exact mode", () => { + vi.stubGlobal("devicePixelRatio", 2); + const { target, displaySizes } = fakeTarget(); + const zoom: SurfaceZoom = { zoom: 1.5, mode: "exact" }; + driveSurfaceResize(target, container(400, 300), () => zoom); + // 120 * 1.5, independent of the 2x display. + expect(displaySizes).toEqual([[800, 600, 180, 240]]); + }); + + it("re-applies the last box when the zoom changes under it", () => { + const { target, displaySizes } = fakeTarget(); + let zoom: SurfaceZoom = { zoom: 1, mode: "relative" }; + const driver = driveSurfaceResize(target, container(800, 600), () => zoom); + displaySizes.length = 0; + + // The box has not moved, so the observer will never fire on its own. + zoom = { zoom: 2, mode: "relative" }; + driver.reapply(); + expect(displaySizes).toEqual([[800, 600, 240, 120]]); + }); + + it("hands the surface back on dispose and goes quiet", () => { + const { target, displaySizes, resizes } = fakeTarget(); + const driver = driveSurfaceResize(target, container(800, 600)); + displaySizes.length = 0; + resizes.length = 0; + + driver.dispose(); + // Null returns the view to frame-tracking mode and withdraws it from the + // server's size mediation. + expect(displaySizes).toEqual([[null, null, null, null]]); + expect(disconnect).toHaveBeenCalledTimes(1); + + // A trailing-edge send must not outlive the mount. + vi.advanceTimersByTime(1000); + expect(resizes).toEqual([]); + // And a late reapply is inert rather than resurrecting the stream. + driver.reapply(); + expect(displaySizes).toHaveLength(1); + }); + + it("ignores a zero-sized box rather than reporting a degenerate one", () => { + const { target, displaySizes } = fakeTarget(); + driveSurfaceResize(target, container(0, 0)); + expect(displaySizes).toEqual([]); + resizeTo(0, 0); + expect(displaySizes).toEqual([]); + // A real box still lands once layout runs. + resizeTo(640, 480); + expect(displaySizes).toEqual([[640, 480, 120, 120]]); + }); +}); diff --git a/js/core/src/__tests__/wheel.test.ts b/js/core/src/__tests__/wheel.test.ts index d46f184c..129e7cd5 100644 --- a/js/core/src/__tests__/wheel.test.ts +++ b/js/core/src/__tests__/wheel.test.ts @@ -1,5 +1,5 @@ import { describe, it, expect } from "vitest"; -import { WheelDetents, WHEEL_DETENT_PX } from "../wheel"; +import { WheelDetents, WHEEL_DETENT_PX, notchedRows } from "../wheel"; /** A wheel event as a browser would report it; jsdom's WheelEvent drops * deltaMode from the init dict on some versions, so build it by hand. */ @@ -71,3 +71,43 @@ describe("WheelDetents", () => { expect(take(w, wheel(1, 2), 0)).toBe(13); }); }); + +describe("notchedRows", () => { + it("moves a whole number of rows, the same for every notch", () => { + // 120px over a 19px cell is 6.3 rows. Left as pixels the remainder is + // what the scroll sync jerks back once the gesture settles. + expect(notchedRows(wheel(WHEEL_DETENT_PX), 19)).toBe(6); + expect(notchedRows(wheel(-WHEEL_DETENT_PX), 19)).toBe(-6); + }); + + it("keeps the browser's own speed where the cell divides the notch", () => { + expect(notchedRows(wheel(WHEEL_DETENT_PX), 10)).toBe(12); + expect(notchedRows(wheel(WHEEL_DETENT_PX), 20)).toBe(6); + }); + + it("gives a coalesced spin the same rows per notch as a single one", () => { + expect(notchedRows(wheel(WHEEL_DETENT_PX * 3), 19)).toBe(18); + }); + + it("reads a Firefox line-mode notch the same as a Chrome one", () => { + expect(notchedRows(wheel(3, 1), 19)).toBe(6); + expect(notchedRows(wheel(-3, 1), 19)).toBe(-6); + }); + + it("leaves a trackpad to the browser", () => { + // Pixel-precise travel means the fraction it reports, and a continuous + // gesture settles once rather than once per notch. + expect(notchedRows(wheel(-4), 19)).toBe(0); + expect(notchedRows(wheel(-53.5), 19)).toBe(0); + expect(notchedRows(wheel(-119), 19)).toBe(0); + }); + + it("leaves a sideways swipe and a page-mode wheel alone", () => { + expect(notchedRows(wheel(0), 19)).toBe(0); + expect(notchedRows(wheel(1, 2), 19)).toBe(0); + }); + + it("never rounds a notch away, however tall the row", () => { + expect(notchedRows(wheel(WHEEL_DETENT_PX), 400)).toBe(1); + }); +}); diff --git a/js/core/src/bsp/index.ts b/js/core/src/bsp/index.ts index ffae3c28..57e99640 100644 --- a/js/core/src/bsp/index.ts +++ b/js/core/src/bsp/index.ts @@ -24,6 +24,7 @@ export { diffAssignment, parseDiffArg, commitAssignment, + manageAssignment, isTileAssignment, parseTileAssignment, webAssignment, diff --git a/js/core/src/bsp/layout.ts b/js/core/src/bsp/layout.ts index bff24ce6..ba5d0616 100644 --- a/js/core/src/bsp/layout.ts +++ b/js/core/src/bsp/layout.ts @@ -73,6 +73,7 @@ const EDITOR_PREFIX = "editor:"; const DIFF_PREFIX = "diff:"; const COMMIT_PREFIX = "commit:"; const PREVIEW_PREFIX = "preview:"; +const MANAGE_PREFIX = "manage:"; /** BSP assignment for an editor tile: "editor::". */ export function editorAssignment(connectionId: string, path: string): string { @@ -127,6 +128,21 @@ export function parseDiffArg(arg: string): { return { side: "unstaged", staged: false, path: arg }; } +/** BSP assignment for a server's own panels — what its session supervisor + * runs, who is connected, its units, its extensions: "manage::". + * + * The trailing colon is not decoration: `parseTileAssignment` splits on the + * first ":" after the prefix, and a manage tile has nothing to say after its + * connection. Keeping the shape means every kind-agnostic path (the hash + * writer, the tab registry, drop handling) treats it like any other tile. + * + * There is one per connection by construction, so opening Manage twice lands + * on the same tile rather than accumulating panels that each hold a live + * client watch. */ +export function manageAssignment(connectionId: string): string { + return `${MANAGE_PREFIX}${connectionId}:`; +} + /** BSP assignment for a commit tile: "commit:::". * `oid` is hex (no ":"), so the first ":" of the arg splits oid from repo. */ export function commitAssignment( @@ -137,14 +153,16 @@ export function commitAssignment( return `${COMMIT_PREFIX}${connectionId}:${oid}:${repoPath}`; } -/** True when the assignment is an editor/diff/commit tile (not a session). */ +/** True when the assignment is an editor/diff/commit/manage tile (not a + * session). */ export function isTileAssignment(value: string | null): boolean { return ( value != null && (value.startsWith(EDITOR_PREFIX) || value.startsWith(DIFF_PREFIX) || value.startsWith(COMMIT_PREFIX) || - value.startsWith(PREVIEW_PREFIX)) + value.startsWith(PREVIEW_PREFIX) || + value.startsWith(MANAGE_PREFIX)) ); } @@ -160,9 +178,10 @@ export function isContentAssignment(value: string | null): boolean { } export interface TileAssignment { - kind: "editor" | "diff" | "commit" | "preview"; + kind: "editor" | "diff" | "commit" | "preview" | "manage"; connectionId: string; - /** Verbatim argument (a path, or ":" for commit). */ + /** Verbatim argument (a path, ":" for commit, empty for + * manage — the connection is the whole address). */ arg: string; } @@ -184,6 +203,9 @@ export function parseTileAssignment( } else if (value != null && value.startsWith(PREVIEW_PREFIX)) { kind = "preview"; prefix = PREVIEW_PREFIX; + } else if (value != null && value.startsWith(MANAGE_PREFIX)) { + kind = "manage"; + prefix = MANAGE_PREFIX; } else { return null; } diff --git a/js/core/src/index.ts b/js/core/src/index.ts index 87e340e2..490380a1 100644 --- a/js/core/src/index.ts +++ b/js/core/src/index.ts @@ -38,6 +38,9 @@ export { wrappingTimestampDelta, } from "./SurfaceStore"; +export { clampZoom, driveSurfaceResize } from "./surfaceResize"; +export type { SurfaceResizeTarget, SurfaceZoom } from "./surfaceResize"; + export { measureCell, cssFontFamily } from "./measure"; export type { CellMetrics } from "./measure"; diff --git a/js/core/src/protocol.ts b/js/core/src/protocol.ts index e159c841..682ec770 100644 --- a/js/core/src/protocol.ts +++ b/js/core/src/protocol.ts @@ -57,6 +57,9 @@ import { CREATE2_HAS_COMMAND, CREATE2_HAS_CWD, CREATE2_WANT_STATUS, + CREATE2_HAS_DEADLINE, + CREATE2_HAS_ENV, + CREATE2_HAS_ARGV, } from "./types"; const textEncoder = new TextEncoder(); @@ -300,18 +303,43 @@ export function buildSearchMessage( return msg; } +export type Create2Options = { + tag?: string; + /** Run this through the server's login shell. Mutually exclusive with + * {@link argv}. */ + command?: string; + /** Exec this argv directly, no shell. Only pass it when the server + * advertised `FEATURE_CREATE_EXEC`; an older one ignores the flag and + * spawns a plain interactive shell instead of what was asked for. */ + argv?: readonly string[]; + srcPtyId?: number; + cwd?: string; + /** Environment overrides, applied on top of everything the server derives. + * Only pass this when the server advertised `FEATURE_CREATE_EXEC`. */ + env?: + | Readonly> + | readonly (readonly [string, string])[]; + /** Only pass this when the server advertised `FEATURE_PTY_DEADLINE`. */ + deadlineMs?: number; + /** Only pass this when the server advertised `FEATURE_CREATE_STATUS`. */ + wantStatus?: boolean; +}; + +/** Encode a `C2S_CREATE2`. + * + * Field order is load-bearing and matches the server's parser: tag, + * `src_pty_id`, cwd, deadline, env, argv, then the command — which has no + * length prefix and therefore has to be last. + * + * Every optional field past the cwd needs its feature bit negotiated first. + * An older server does not reject an unknown `features` bit; it ignores the + * bit, does not skip the bytes, and reads them as the start of the command. + * See the constants in `types.ts` for what each one does when unsupported. */ export function buildCreate2Message( nonce: number, rows: number, cols: number, - options?: { - tag?: string; - command?: string; - srcPtyId?: number; - cwd?: string; - /** Only pass this when the server advertised `FEATURE_CREATE_STATUS`. */ - wantStatus?: boolean; - }, + options?: Create2Options, ): Uint8Array { const tagBytes = options?.tag ? textEncoder.encode(options.tag) @@ -324,10 +352,28 @@ export function buildCreate2Message( ? rawCwdBytes.subarray(0, Math.min(rawCwdBytes.length, 0xffff)) : new Uint8Array(0); const hasCwd = cwdBytes.length > 0; + const argv = options?.argv ?? null; + if (argv && options?.command) { + throw new Error("buildCreate2Message: argv and command are exclusive"); + } + if (argv && argv.length === 0) { + throw new Error("buildCreate2Message: argv is empty"); + } + const argvBytes = argv?.map((arg) => textEncoder.encode(arg)) ?? null; + const env = normalizeEnv(options?.env); + const envBytes = env.map( + ([key, value]) => + [textEncoder.encode(key), textEncoder.encode(value)] as const, + ); + const deadlineMs = options?.deadlineMs; + const hasDeadline = deadlineMs != null && deadlineMs > 0; const cmdText = options?.command?.trim() ?? ""; const hasCmd = cmdText.length > 0; if (hasSrc) features |= CREATE2_HAS_SRC_PTY; if (hasCwd) features |= CREATE2_HAS_CWD; + if (hasDeadline) features |= CREATE2_HAS_DEADLINE; + if (envBytes.length) features |= CREATE2_HAS_ENV; + if (argvBytes) features |= CREATE2_HAS_ARGV; if (hasCmd) features |= CREATE2_HAS_COMMAND; if (options?.wantStatus) features |= CREATE2_WANT_STATUS; const cmdBytes = hasCmd ? textEncoder.encode(cmdText) : new Uint8Array(0); @@ -336,6 +382,13 @@ export function buildCreate2Message( tagBytes.length + (hasSrc ? 2 : 0) + (hasCwd ? 2 + cwdBytes.length : 0) + + (hasDeadline ? 4 : 0) + + (envBytes.length + ? 2 + envBytes.reduce((n, [k, v]) => n + 2 + k.length + 4 + v.length, 0) + : 0) + + (argvBytes + ? 2 + argvBytes.reduce((n, arg) => n + 4 + arg.length, 0) + : 0) + cmdBytes.length, ); msg[0] = C2S_CREATE2; @@ -365,10 +418,79 @@ export function buildCreate2Message( msg.set(cwdBytes, cursor); cursor += cwdBytes.length; } + if (hasDeadline) { + const ms = Math.min(deadlineMs!, 0xffffffff) >>> 0; + msg[cursor] = ms & 0xff; + msg[cursor + 1] = (ms >>> 8) & 0xff; + msg[cursor + 2] = (ms >>> 16) & 0xff; + msg[cursor + 3] = (ms >>> 24) & 0xff; + cursor += 4; + } + if (envBytes.length) { + msg[cursor] = envBytes.length & 0xff; + msg[cursor + 1] = (envBytes.length >> 8) & 0xff; + cursor += 2; + for (const [key, value] of envBytes) { + msg[cursor] = key.length & 0xff; + msg[cursor + 1] = (key.length >> 8) & 0xff; + cursor += 2; + msg.set(key, cursor); + cursor += key.length; + msg[cursor] = value.length & 0xff; + msg[cursor + 1] = (value.length >>> 8) & 0xff; + msg[cursor + 2] = (value.length >>> 16) & 0xff; + msg[cursor + 3] = (value.length >>> 24) & 0xff; + cursor += 4; + msg.set(value, cursor); + cursor += value.length; + } + } + if (argvBytes) { + msg[cursor] = argvBytes.length & 0xff; + msg[cursor + 1] = (argvBytes.length >> 8) & 0xff; + cursor += 2; + for (const arg of argvBytes) { + msg[cursor] = arg.length & 0xff; + msg[cursor + 1] = (arg.length >>> 8) & 0xff; + msg[cursor + 2] = (arg.length >>> 16) & 0xff; + msg[cursor + 3] = (arg.length >>> 24) & 0xff; + cursor += 4; + msg.set(arg, cursor); + cursor += arg.length; + } + } if (cmdBytes.length) msg.set(cmdBytes, cursor); return msg; } +/** Accept either an object or entry pairs, and reject what the server would. + * A key carrying `=` or a NUL cannot survive `execve`, and a duplicate has no + * resolution that does not silently discard a value. */ +function normalizeEnv( + env: Create2Options["env"], +): (readonly [string, string])[] { + if (!env) return []; + const entries = Array.isArray(env) + ? (env as (readonly [string, string])[]) + : Object.entries(env as Record); + const seen = new Set(); + for (const [key, value] of entries) { + if (!key || key.includes("=") || key.includes("\0")) { + throw new Error( + `buildCreate2Message: bad environment key ${JSON.stringify(key)}`, + ); + } + if (value.includes("\0")) { + throw new Error(`buildCreate2Message: NUL in value for ${key}`); + } + if (seen.has(key)) { + throw new Error(`buildCreate2Message: duplicate environment key ${key}`); + } + seen.add(key); + } + return entries; +} + /** Mouse event types for C2S_MOUSE. */ export const MOUSE_DOWN = 0; export const MOUSE_UP = 1; diff --git a/js/core/src/surfaceResize.ts b/js/core/src/surfaceResize.ts new file mode 100644 index 00000000..54e2bb24 --- /dev/null +++ b/js/core/src/surfaceResize.ts @@ -0,0 +1,227 @@ +/** + * The resizable-pane half of a surface view, shared by every framework + * binding. + * + * A surface view comes in two shapes. A *passive* one — a dock card, a + * switcher preview — is handed a box and asks the server for a fixed + * downscale of whatever the surface happens to be; `BlitSurfaceCanvas` + * measures that itself. A *resizable* one owns its surface's size: it + * reports its box as a display size, which is what puts the view into the + * server's size mediation and, incidentally, what every input path is gated + * on (`_displaySize`). Without it a view is an inert preview: no pointer, no + * wheel, no keyboard, no IME, and a 15 fps thumbnail-grade stream. + * + * That half used to live in each binding, which is how the React one ended up + * never calling {@link SurfaceResizeTarget.setDisplaySize} at all — a + * documented, full-window React pane that could not be clicked. One + * implementation, two thin call sites. + */ + +/** The part of `BlitSurfaceCanvas` a resize driver needs. */ +export interface SurfaceResizeTarget { + /** Report the box, in device pixels, this view is rendering into. */ + setDisplaySize( + width: number | null, + height?: number, + scale120?: number, + cssScale120?: number, + ): void; + /** Ask the server to resize the surface to this view's box. */ + requestResize(width: number, height: number, scale120: number): void; +} + +export interface SurfaceZoom { + /** Zoom factor; see {@link SurfaceZoom.mode}. */ + zoom?: number; + /** `relative` multiplies the display's DPI by `zoom`; `exact` uses `zoom` + * as the absolute surface scale, independent of display DPI. */ + mode?: "relative" | "exact"; +} + +/** + * Clamp to a range that stays useful at both ends: below 0.25 an app is handed + * a logical size most toolkits refuse to lay out, and above 4 one pane's + * demand for scale would dominate every co-viewer's stream. + */ +export function clampZoom(zoom: number | undefined): number { + if (typeof zoom !== "number" || !Number.isFinite(zoom) || zoom <= 0) return 1; + return Math.min(4, Math.max(0.25, zoom)); +} + +/** + * Short, because the server coalesces on its own: a configure opens a settle + * window there and every size that lands inside it is folded into one + * configure at the end. A long trailing edge here doesn't save the compositor + * anything, it just delays the last size — and some layout changes are two box + * changes in quick succession rather than a drag. Restoring a parked surface + * is one: the pane appears, then widens again as the dock the card left + * closes, and the second size used to sit here for 100 ms while the server + * built an encoder for the first. + */ +const RESIZE_DEBOUNCE_MS = 30; + +/** + * If no resize event for this long, the next one is treated as the start of a + * fresh drag and fires immediately — so each user-visible drag gets a + * leading-edge dispatch and the perceived reaction is bounded by RTT rather + * than the trailing-edge debounce. + */ +const DRAG_GAP_MS = 250; + +function fallbackScale120(): number { + return Math.round((globalThis.devicePixelRatio || 1) * 120); +} + +/** An entry's content box in device pixels, or null when it does not report + * one. Deliberately the exact `devicePixelContentBoxSize` here, unlike + * `downscale.devicePixelBox`: a pane wants the size it will actually be + * rasterised at, and the octave quantisation that makes the approximation + * fine for a thumbnail does not apply. */ +function devicePixelSize( + entry: ResizeObserverEntry, +): { width: number; height: number } | null { + const box = entry.devicePixelContentBoxSize; + const size = Array.isArray(box) ? box[0] : box; + if (!size) return null; + const width = Math.round(size.inlineSize); + const height = Math.round(size.blockSize); + return width > 0 && height > 0 ? { width, height } : null; +} + +/** + * Drive `target`'s display size and server-side resizes from `container`'s box. + * + * `getZoom` is read on every measurement rather than captured, so a binding can + * change the zoom without tearing the driver down — rebuilding it would + * unsubscribe the view and cost a keyframe. Call {@link reapply} when the zoom + * changes: the box has not moved, so the observer will never fire on its own. + */ +export function driveSurfaceResize( + target: SurfaceResizeTarget, + container: HTMLElement, + getZoom: () => SurfaceZoom = () => ({}), +): { reapply(): void; dispose(): void } { + /** The last box the observer reported, so a zoom change can be re-applied + * without waiting for the container to change size — it never will. */ + let lastBox: { + cssW: number; + cssH: number; + physicalW?: number; + physicalH?: number; + } | null = null; + + let resizeTimer: ReturnType | undefined; + /** Negative infinity rather than 0, so the *first* box a view ever reports + * always counts as a drag start and dispatches at wire speed. Anchoring at + * 0 made that depend on when in the page's life the view happened to mount: + * within the first {@link DRAG_GAP_MS} of load the first size waited out the + * debounce, and after it did not. */ + let lastResizeAt = Number.NEGATIVE_INFINITY; + let lastSentW = 0; + let lastSentH = 0; + let lastSentScale120 = 0; + let disposed = false; + + const send = (w: number, h: number, scale120: number) => { + if (w === lastSentW && h === lastSentH && scale120 === lastSentScale120) + return; + lastSentW = w; + lastSentH = h; + lastSentScale120 = scale120; + target.requestResize(w, h, scale120); + }; + + const applySize = ( + cssW: number, + cssH: number, + physicalW?: number, + physicalH?: number, + ) => { + if (disposed) return; + // Even, because the encoder rounds each axis *down* to even on its own + // (H.264/HEVC/AV1 NV12 sampling grids). Asking for an odd extent means the + // frame comes back a pixel short of the pane on that axis only, so the + // aspect no longer matches and `object-fit: contain` letterboxes the + // difference. Giving up the odd pixel here costs nothing — it was never + // going to carry image — and makes the server's rounding a no-op. + const even = (n: number) => Math.max(2, n - (n % 2)); + const dpr = (globalThis.devicePixelRatio || 1) as number; + const w = even(Math.round(physicalW ?? cssW * dpr)); + const h = even(Math.round(physicalH ?? cssH * dpr)); + if (w <= 0 || h <= 0) return; + // The container's measured device-pixel ratio, which is what converts the + // canvas's device pixels back to a CSS box. + const cssScale120 = + cssW > 0 && cssH > 0 + ? Math.round(((w / cssW + h / cssH) / 2) * 120) + : fallbackScale120(); + const { zoom, mode } = getZoom(); + const factor = clampZoom(zoom); + // The pane always holds `w x h` device pixels. Relative zoom rides on its + // DPI; exact zoom names the surface scale directly. A sub-1x scale is + // meaningful: the server gives the app a larger logical window, composites + // at Wayland's 1x floor, and downsamples the stream into this pane. + const scale120 = Math.max( + 1, + Math.round((mode === "exact" ? 120 : cssScale120) * factor), + ); + target.setDisplaySize(w, h, scale120, cssScale120); + lastBox = { cssW, cssH, physicalW, physicalH }; + const now = performance.now(); + const isDragStart = now - lastResizeAt > DRAG_GAP_MS; + lastResizeAt = now; + // Leading edge: first event of a new interaction dispatches at wire speed + // so the server pipeline (configure -> repaint -> encode) starts as soon as + // possible. + if (isDragStart) send(w, h, scale120); + // Trailing edge: settle on the final size after the interaction ends, in + // case it differs from the leading-edge value. + clearTimeout(resizeTimer); + resizeTimer = setTimeout(() => send(w, h, scale120), RESIZE_DEBOUNCE_MS); + }; + + let observer: ResizeObserver | null = null; + if (typeof ResizeObserver !== "undefined") { + observer = new ResizeObserver((entries) => { + for (const entry of entries) { + const { width, height } = entry.contentRect; + if (width > 0 && height > 0) { + const dpx = devicePixelSize(entry); + applySize(width, height, dpx?.width, dpx?.height); + } + } + }); + try { + observer.observe(container, { box: "device-pixel-content-box" }); + } catch { + observer.observe(container); + } + } + + const rect = container.getBoundingClientRect(); + if (rect.width > 0 && rect.height > 0) { + applySize(rect.width, rect.height); + } + + return { + /** Re-apply the last box under the current zoom. Goes through + * `applySize`, so it takes the same debounce and de-duplication as a + * drag. */ + reapply(): void { + if (disposed || !lastBox) return; + applySize( + lastBox.cssW, + lastBox.cssH, + lastBox.physicalW, + lastBox.physicalH, + ); + }, + dispose(): void { + if (disposed) return; + disposed = true; + clearTimeout(resizeTimer); + observer?.disconnect(); + target.setDisplaySize(null); + }, + }; +} diff --git a/js/core/src/types.ts b/js/core/src/types.ts index 73ce9bde..b32986cc 100644 --- a/js/core/src/types.ts +++ b/js/core/src/types.ts @@ -241,6 +241,11 @@ export interface BlitConnectionSnapshot { supportsExtensions: boolean; /** Server understands viewer media, portals, and MPRIS runtime state. */ supportsDesktopMedia: boolean; + /** A terminal can be started the way a process is: an exact argv exec'd + * without a login shell, plus environment overrides. Must be checked + * before asking — an older server ignores those fields rather than + * refusing them, and quietly starts something else. */ + supportsCreateExec: boolean; retryCount: number; /** Opaque 64-bit identifier for the current server process, or `null` for * servers predating the extended HELLO. */ @@ -375,6 +380,21 @@ export const CREATE2_HAS_CWD = 1 << 2; * ignores the bit and answers a refusal with nothing at all, leaving the * create pending forever. */ export const CREATE2_WANT_STATUS = 1 << 3; +/** Arm a server-enforced deadline at creation: `[ms:4]`, after any cwd and + * before any command bytes. Only set it when HELLO advertised + * {@link FEATURE_PTY_DEADLINE} — an older server does not know to skip the + * four bytes and reads them as the start of the command. */ +export const CREATE2_HAS_DEADLINE = 1 << 4; +/** Environment overrides for the child: `[count:2]` then `count` records of + * `[key_len:2][key:N][value_len:4][value:N]`, applied on top of everything + * the server derives. Needs {@link FEATURE_CREATE_EXEC}: an older server + * ignores the bit, does not skip the block, and runs it as command text. */ +export const CREATE2_HAS_ENV = 1 << 5; +/** Exec an argv directly, no shell: `[argc:2]` then `argc` records of + * `[len:4][arg:N]`. Mutually exclusive with {@link CREATE2_HAS_COMMAND}. + * Needs {@link FEATURE_CREATE_EXEC}: an older server ignores the bit, finds + * no command, and spawns a plain interactive shell instead. */ +export const CREATE2_HAS_ARGV = 1 << 6; /** Wire protocol constants: server-to-client message types. */ export const S2C_UPDATE = 0x00; @@ -627,6 +647,15 @@ export const FEATURE_CLIENT_CONTROL = 1 << 20; * and answer it with {@link S2C_CLIENT_LIST2}. Implies * {@link FEATURE_CLIENT_CONTROL}. */ export const FEATURE_CLIENT_ORIGIN = 1 << 27; +/** `C2S_CREATE2` accepts {@link CREATE2_HAS_ARGV} and {@link CREATE2_HAS_ENV}, + * so a terminal can be started the way a native process is: an exact argv + * exec'd without a shell, plus environment overrides. + * + * Neither flag is probeable — an older server does not refuse an unknown + * `features` bit, it ignores the bit and misreads the bytes that follow — so + * this has to be negotiated rather than discovered. Not advertised on + * Windows servers, where the pseudoconsole path can honor neither. */ +export const FEATURE_CREATE_EXEC = 1 << 29; // -- Common status registry (docs/protocol.md) ------------------------------ // diff --git a/js/core/src/wheel.ts b/js/core/src/wheel.ts index 8f4a65b5..ee67391d 100644 --- a/js/core/src/wheel.ts +++ b/js/core/src/wheel.ts @@ -37,6 +37,47 @@ export const SCROLL_STOP_MS = 280; * page-mode wheel on a very tall pane) can't flood the PTY. */ const MAX_DETENTS_PER_EVENT = 32; +/** + * Whole rows a notched wheel should travel, or 0 for anything that is not a + * notched wheel and should keep the browser's own scrolling. + * + * A notch is 120 CSS px whatever the font is, so left to the browser it lands + * mid-row. A terminal can only show whole rows, so the offset that position + * maps to is rounded, and the render loop writes the rounding back to + * `scrollTop` once the gesture settles — a jerk of up to half a row, in + * whichever direction the remainder fell, arriving as late as the next cursor + * blink. Every notch leaves a different remainder, so the jerks alternate: + * at a 19px cell, twelve notches moved 6 or 7 rows apiece and snapped back by + * +6, -7, -1, +5, -8, -2, +4, -9, -3, +3, +9, -4 px. + * + * Rounding the *travel* instead keeps the surface on the row grid, so there is + * no remainder to write back and every notch moves the same distance. The + * distance is still the notch's own 120px worth of rows, so the wheel keeps + * the speed the browser was giving it. + * + * Pixel-precise devices are deliberately left alone: a trackpad means the + * fraction it reports, and being continuous it settles once per gesture rather + * than once per notch. macOS varies a notch's size with its own scroll + * acceleration, which is why a wheel there is not recognisable by size and + * falls here too. + */ +export function notchedRows(e: WheelEvent, rowHeightPx: number): number { + if (!(rowHeightPx > 0)) return 0; + const dy = e.deltaY; + if (!dy || !Number.isFinite(dy)) return 0; + const rowsPerNotch = Math.max(1, Math.round(WHEEL_DETENT_PX / rowHeightPx)); + // Firefox reports a notched wheel in lines of its own line box; Chrome and + // Edge report it on a whole-detent pixel grid. Anything else is travel. + const notches = + e.deltaMode === WHEEL_MODE_LINE + ? dy / WHEEL_LINES_PER_DETENT + : e.deltaMode === 0 + ? dy / WHEEL_DETENT_PX + : 0; + if (!Number.isInteger(notches) || notches === 0) return 0; + return notches * rowsPerNotch; +} + /** * Accumulates wheel travel into whole detents. * diff --git a/js/react/src/BlitSurfaceView.tsx b/js/react/src/BlitSurfaceView.tsx index 7a690940..25510c13 100644 --- a/js/react/src/BlitSurfaceView.tsx +++ b/js/react/src/BlitSurfaceView.tsx @@ -1,5 +1,15 @@ -import { forwardRef, useEffect, useImperativeHandle, useRef } from "react"; -import { BlitSurfaceCanvas } from "@blit-sh/core"; +import { + forwardRef, + useEffect, + useImperativeHandle, + useRef, + useState, +} from "react"; +import { + BlitSurfaceCanvas, + detectCodecSupport, + driveSurfaceResize, +} from "@blit-sh/core"; import type { BlitSurface, ConnectionId, @@ -16,6 +26,31 @@ export interface BlitSurfaceViewProps { live?: boolean; /** How touchscreen contacts are delivered. Defaults to pointer emulation. */ touchMode?: SurfaceTouchMode; + /** + * Whether this view owns its surface's size, resizing it to fill the + * container. Defaults to true. + * + * Pass `false` only for a passive preview — a dock card, a switcher + * thumbnail — that shares another view's stream. Such a view is served a + * fixed downscale capped at a thumbnail cadence and takes no input at all: + * every pointer, wheel, keyboard and IME path is gated on having a display + * size. + */ + resizable?: boolean; + /** + * Surface zoom factor, e.g. 1.25 for 125% or an exact 1.25x scale. + * + * How this value is interpreted is controlled by `zoomMode`. Defaults to + * 1. Only resizable views drive the surface's scale, so it has no effect + * elsewhere. + */ + zoom?: number; + /** + * `relative` multiplies the display's DPI by `zoom`; `exact` uses `zoom` as + * the absolute surface scale, independent of display DPI. Defaults to + * `relative`. + */ + zoomMode?: "relative" | "exact"; } export interface BlitSurfaceViewHandle { @@ -27,12 +62,32 @@ export const BlitSurfaceView = forwardRef< BlitSurfaceViewHandle, BlitSurfaceViewProps >(function BlitSurfaceView( - { connectionId, surfaceId, className, style, live, touchMode }, + { + connectionId, + surfaceId, + className, + style, + live, + touchMode, + resizable, + zoom, + zoomMode, + }, ref, ) { const workspace = useRequiredBlitWorkspace(); const containerRef = useRef(null); const canvasRef = useRef(null); + // State, not a ref, so the resize effect below re-runs when the canvas is + // rebuilt without having to restate the mount effect's dependencies. + const [mounted, setMounted] = useState(null); + + // The driver reads zoom on every measurement rather than capturing it, so a + // zoom change does not have to tear the observer down — that would + // unsubscribe the view and cost a keyframe. + const zoomRef = useRef({ zoom, zoomMode }); + zoomRef.current = { zoom, zoomMode }; + const driverRef = useRef<{ reapply(): void } | null>(null); useImperativeHandle(ref, () => ({ get canvas() { @@ -46,29 +101,68 @@ export const BlitSurfaceView = forwardRef< useEffect(() => { const container = containerRef.current; if (!container) return; + // Unconditional: a page showing only passive previews still needs the + // probe, or its subscribes carry codec_support=0 ("accept anything") + // forever. + detectCodecSupport(); const surface = new BlitSurfaceCanvas({ workspace, connectionId, surfaceId, live, + resizable: resizable !== false, touchMode, }); surface.attach(container); canvasRef.current = surface; + setMounted(surface); return () => { + setMounted(null); surface.dispose(); canvasRef.current = null; }; - // `touchMode` is deliberately absent: it is applied below instead. Listing - // it here would tear down and rebuild the decoder and the server-side - // stream on a setting that only changes which opcode input events use. + // `touchMode` and `zoom`/`zoomMode` are deliberately absent: they are + // applied below instead. Listing them here would tear down and rebuild the + // decoder and the server-side stream on settings that only change which + // opcode input events use, or what scale the surface is asked for. // eslint-disable-next-line react-hooks/exhaustive-deps - }, [workspace, connectionId, surfaceId, live]); + }, [workspace, connectionId, surfaceId, live, resizable]); useEffect(() => { canvasRef.current?.setTouchMode(touchMode ?? "direct"); }, [touchMode]); + // Own the surface's size while resizable. The policy — even extents, the + // leading/trailing-edge resize debounce, the zoom modes — lives in core so + // every binding drives it identically; this only wires it to React's + // lifecycle. The canvas resolution is set immediately via setDisplaySize so + // there is no CSS-scaling gap while waiting for the Wayland app to resize. + useEffect(() => { + const container = containerRef.current; + if (resizable === false || !mounted || !container) return; + const driver = driveSurfaceResize(mounted, container, () => ({ + zoom: zoomRef.current.zoom, + mode: zoomRef.current.zoomMode, + })); + driverRef.current = driver; + return () => { + driverRef.current = null; + driver.dispose(); + }; + }, [mounted, resizable]); + + // Tracks the zoom controls only. The box has not moved, so the observer will + // never fire on its own. Skips the mount run — the driver has already + // applied the initial box with these values. + const zoomApplied = useRef(false); + useEffect(() => { + if (!zoomApplied.current) { + zoomApplied.current = true; + return; + } + driverRef.current?.reapply(); + }, [zoom, zoomMode]); + return (
{ + const actual = + await vi.importActual("@blit-sh/core"); + return { + ...actual, + detectCodecSupport: vi.fn(), + BlitSurfaceCanvas: class { + canvasElement = null; + surfaceInfo = undefined; + constructor(options: { resizable?: boolean }) { + constructedResizable = options.resizable; + } + attach = mockAttach; + dispose = mockDispose; + setTouchMode = mockSetTouchMode; + setDisplaySize = mockSetDisplaySize; + requestResize = mockRequestResize; + }, + }; +}); + +const wasm = { + Terminal: class {}, +} as unknown as BlitWasmModule; + +function setup() { + const transport = new MockTransport(); + const workspace = new BlitWorkspace({ + wasm, + connections: [{ id: "c1", transport }], + }); + return { workspace }; +} + +function renderView(props: Record = {}) { + const { workspace } = setup(); + return render( + + + , + ); +} + +describe("BlitSurfaceView", () => { + beforeEach(() => { + vi.clearAllMocks(); + constructedResizable = undefined; + // jsdom reports a 0x0 box, which the driver correctly ignores. Give every + // container a real one so the initial measurement lands. + vi.spyOn(HTMLElement.prototype, "getBoundingClientRect").mockReturnValue({ + width: 800, + height: 600, + left: 0, + top: 0, + right: 800, + bottom: 600, + } as DOMRect); + vi.stubGlobal("devicePixelRatio", 1); + vi.stubGlobal( + "ResizeObserver", + class { + observe = vi.fn(); + disconnect = vi.fn(); + }, + ); + }); + + afterEach(() => { + cleanup(); + vi.unstubAllGlobals(); + vi.restoreAllMocks(); + }); + + it("owns its surface's size by default", () => { + // Regression: this component never called setDisplaySize, so a documented + // full-window React pane was an inert 15fps preview — every pointer, + // wheel, keyboard and IME path in the canvas is gated on having one. + renderView(); + expect(constructedResizable).toBe(true); + expect(mockSetDisplaySize).toHaveBeenCalledWith(800, 600, 120, 120); + expect(mockRequestResize).toHaveBeenCalledWith(800, 600, 120); + }); + + it("stays a passive preview when asked to", () => { + renderView({ resizable: false }); + expect(constructedResizable).toBe(false); + expect(mockSetDisplaySize).not.toHaveBeenCalled(); + expect(mockRequestResize).not.toHaveBeenCalled(); + }); + + it("applies a relative zoom to the surface scale", () => { + renderView({ zoom: 1.5 }); + // The pane still holds 800x600 device pixels; only the scale moves. + expect(mockSetDisplaySize).toHaveBeenCalledWith(800, 600, 180, 120); + }); + + it("re-applies the box when zoom changes, without rebuilding the canvas", () => { + const { rerender } = renderView({ zoom: 1 }); + const { workspace } = setup(); + mockSetDisplaySize.mockClear(); + + rerender( + + + , + ); + // A zoom change must not tear the stream down: no new canvas, no dispose. + expect(mockSetDisplaySize).toHaveBeenCalledWith(800, 600, 240, 120); + }); + + it("hands the surface back on unmount", () => { + const { unmount } = renderView(); + mockSetDisplaySize.mockClear(); + unmount(); + expect(mockSetDisplaySize).toHaveBeenCalledWith(null); + expect(mockDispose).toHaveBeenCalled(); + }); +}); diff --git a/js/solid/src/BlitSurfaceView.tsx b/js/solid/src/BlitSurfaceView.tsx index 9ef875ec..e778e2e3 100644 --- a/js/solid/src/BlitSurfaceView.tsx +++ b/js/solid/src/BlitSurfaceView.tsx @@ -8,7 +8,11 @@ import { Show, type JSX, } from "solid-js"; -import { BlitSurfaceCanvas, detectCodecSupport } from "@blit-sh/core"; +import { + BlitSurfaceCanvas, + detectCodecSupport, + driveSurfaceResize, +} from "@blit-sh/core"; import type { ConnectionId, SurfaceTouchMode } from "@blit-sh/core"; import { useRequiredBlitWorkspace } from "./BlitContext"; @@ -19,7 +23,16 @@ export interface BlitSurfaceViewProps { style?: JSX.CSSProperties; /** When true the inner canvas is focused so it receives keyboard input. */ focus?: boolean; - /** When true the surface is resized to fill the container. */ + /** + * Whether this view owns its surface's size, resizing it to fill the + * container. Defaults to true. + * + * Pass `false` only for a passive preview — a dock card, a switcher + * thumbnail — that shares another view's stream. Such a view is served a + * fixed downscale capped at a thumbnail cadence and takes no input at all: + * every pointer, wheel, keyboard and IME path is gated on having a display + * size. + */ resizable?: boolean; /** * When false, render only frames already present in the shared cache and @@ -44,21 +57,19 @@ export interface BlitSurfaceViewProps { zoomMode?: "relative" | "exact"; } -/** Clamp to a range that stays useful at both ends: below 0.25 an app is - * handed a logical size most toolkits refuse to lay out, and above 4 one - * pane's demand for scale would dominate every co-viewer's stream. */ -function clampZoom(zoom: number | undefined): number { - if (typeof zoom !== "number" || !Number.isFinite(zoom) || zoom <= 0) return 1; - return Math.min(4, Math.max(0.25, zoom)); -} - export function BlitSurfaceView(props: BlitSurfaceViewProps) { const workspace = useRequiredBlitWorkspace(); + /** Interactive unless explicitly opted out; see + * {@link BlitSurfaceViewProps.resizable}. */ + const resizable = () => props.resizable !== false; let containerRef!: HTMLDivElement; const [mounted, setMounted] = createSignal(null); const [videoError, setVideoError] = createSignal(null); onMount(() => { + // Unconditional: a page showing only passive previews still needs the probe, + // or its subscribes carry codec_support=0 ("accept anything") forever. + detectCodecSupport(); const conn = workspace.getConnection(props.connectionId); if (conn?.surfaceStore.videoUnavailableReason) { setVideoError(conn.surfaceStore.videoUnavailableReason); @@ -68,7 +79,7 @@ export function BlitSurfaceView(props: BlitSurfaceViewProps) { connectionId: props.connectionId, surfaceId: props.surfaceId, live: props.live, - resizable: props.resizable, + resizable: resizable(), touchMode: props.touchMode, }); surface.attach(containerRef); @@ -105,163 +116,25 @@ export function BlitSurfaceView(props: BlitSurfaceViewProps) { * current box after the zoom factor changes. */ let reapplyZoom: (() => void) | null = null; - // Observe container size and request a server-side resize when resizable. - // The canvas resolution is set immediately via setDisplaySize so there is - // no CSS-scaling gap while waiting for the Wayland app to resize. - // The server resize request is debounced to avoid flooding the compositor - // with redundant configure cycles and encoder recreations during a - // drag-resize. + // Own the surface's size while resizable. The policy — even extents, the + // leading/trailing-edge resize debounce, the zoom modes — lives in core so + // every binding drives it identically; this only wires it to Solid's + // lifecycle. The canvas resolution is set immediately via setDisplaySize so + // there is no CSS-scaling gap while waiting for the Wayland app to resize. createEffect(() => { const s = mounted(); - if (!props.resizable || !s) return; - - const fallbackScale120 = () => - Math.round((window.devicePixelRatio || 1) * 120); - detectCodecSupport(); - - // Read untracked: a zoom change must not tear this effect down and + if (!resizable() || !s) return; + // Read zoom untracked: a zoom change must not tear this effect down and // rebuild the observer (that unsubscribes the view and costs a keyframe). // The dedicated effect below re-applies the last box instead. - const zoom = () => clampZoom(untrack(() => props.zoom)); - const zoomMode = () => untrack(() => props.zoomMode ?? "relative"); - // The last box the observer reported, so a zoom change can be re-applied - // without waiting for the container to change size — it never will. - let lastBox: { - cssW: number; - cssH: number; - physicalW?: number; - physicalH?: number; - } | null = null; - - let resizeTimer: ReturnType | undefined; - let lastResizeAt = 0; - let lastSentW = 0; - let lastSentH = 0; - let lastSentScale120 = 0; - // Short, because the server coalesces on its own: a configure opens a - // settle window there and every size that lands inside it is folded - // into one configure at the end. A long trailing edge here doesn't - // save the compositor anything, it just delays the last size — and - // some layout changes are two box changes in quick succession rather - // than a drag. Restoring a parked surface is one: the pane appears, - // then widens again as the dock the card left closes, and the second - // size used to sit here for 100 ms while the server built an encoder - // for the first. - const RESIZE_DEBOUNCE_MS = 30; - // If no resize event for this long, the next one is treated as the - // start of a fresh drag and fires immediately — so each user-visible - // drag gets a leading-edge dispatch and the perceived reaction is - // bounded by RTT rather than the trailing-edge debounce. - const DRAG_GAP_MS = 250; - - const send = (w: number, h: number, scale120: number) => { - if (w === lastSentW && h === lastSentH && scale120 === lastSentScale120) - return; - lastSentW = w; - lastSentH = h; - lastSentScale120 = scale120; - s.requestResize(w, h, scale120); - }; - - const applySize = ( - cssW: number, - cssH: number, - physicalW?: number, - physicalH?: number, - ) => { - // Even, because the encoder rounds each axis *down* to even on its own - // (H.264/HEVC/AV1 NV12 sampling grids). Asking for an odd extent means - // the frame comes back a pixel short of the pane on that axis only, so - // the aspect no longer matches and `object-fit: contain` letterboxes - // the difference. Giving up the odd pixel here costs nothing — it was - // never going to carry image — and makes the server's rounding a no-op. - const even = (n: number) => Math.max(2, n - (n % 2)); - const w = even( - Math.round(physicalW ?? cssW * (window.devicePixelRatio || 1)), - ); - const h = even( - Math.round(physicalH ?? cssH * (window.devicePixelRatio || 1)), - ); - if (w <= 0 || h <= 0) return; - // The container's measured device-pixel ratio, which is what converts - // the canvas's device pixels back to a CSS box. - const cssScale120 = - cssW > 0 && cssH > 0 - ? Math.round(((w / cssW + h / cssH) / 2) * 120) - : fallbackScale120(); - // The pane always holds `w × h` device pixels. Relative zoom rides - // on its DPI; exact zoom names the surface scale directly. A sub-1x - // scale is meaningful: the server gives the app a larger logical - // window, composites at Wayland's 1x floor, and downsamples the stream - // into this pane. - const scale120 = Math.max( - 1, - Math.round((zoomMode() === "exact" ? 120 : cssScale120) * zoom()), - ); - s.setDisplaySize(w, h, scale120, cssScale120); - lastBox = { cssW, cssH, physicalW, physicalH }; - const now = performance.now(); - const isDragStart = now - lastResizeAt > DRAG_GAP_MS; - lastResizeAt = now; - // Leading edge: first event of a new interaction dispatches at - // wire speed so the server pipeline (configure → repaint → encode) - // starts as soon as possible. - if (isDragStart) send(w, h, scale120); - // Trailing edge: settle on the final size after the interaction - // ends, in case it differs from the leading-edge value. - clearTimeout(resizeTimer); - resizeTimer = setTimeout(() => send(w, h, scale120), RESIZE_DEBOUNCE_MS); - }; - - const devicePixelSize = (entry: ResizeObserverEntry) => { - const box = entry.devicePixelContentBoxSize; - const size = Array.isArray(box) ? box[0] : box; - if (!size) return null; - const width = Math.round(size.inlineSize); - const height = Math.round(size.blockSize); - return width > 0 && height > 0 ? { width, height } : null; - }; - - const ro = new ResizeObserver((entries) => { - for (const entry of entries) { - const { width, height } = entry.contentRect; - if (width > 0 && height > 0) { - const dpx = devicePixelSize(entry); - applySize(width, height, dpx?.width, dpx?.height); - } - } - }); - try { - ro.observe(containerRef, { box: "device-pixel-content-box" }); - } catch { - ro.observe(containerRef); - } - - const rect = containerRef.getBoundingClientRect(); - if (rect.width > 0 && rect.height > 0) { - applySize(rect.width, rect.height); - } - - // Changing the zoom is a resize as far as the surface is concerned: the - // box is unchanged, so the observer will never fire, but the logical - // size the app is being handed just moved. Re-apply the last box under - // the new factor — through applySize, so it takes the same debounce and - // the same de-duplication as a drag. - reapplyZoom = () => { - if (!lastBox) return; - applySize( - lastBox.cssW, - lastBox.cssH, - lastBox.physicalW, - lastBox.physicalH, - ); - }; - + const driver = driveSurfaceResize(s, containerRef, () => ({ + zoom: untrack(() => props.zoom), + mode: untrack(() => props.zoomMode), + })); + reapplyZoom = () => driver.reapply(); onCleanup(() => { reapplyZoom = null; - clearTimeout(resizeTimer); - ro.disconnect(); - s.setDisplaySize(null); + driver.dispose(); }); }); diff --git a/js/solid/src/__tests__/BlitSurfaceView.test.tsx b/js/solid/src/__tests__/BlitSurfaceView.test.tsx index f7999bbd..72784d30 100644 --- a/js/solid/src/__tests__/BlitSurfaceView.test.tsx +++ b/js/solid/src/__tests__/BlitSurfaceView.test.tsx @@ -193,4 +193,30 @@ describe("BlitSurfaceView zoom", () => { setMode("exact"); expect(mockSetDisplaySize).toHaveBeenLastCalledWith(1600, 1200, 120, 240); }); + + /** A view with no display size takes no input at all — every pointer, wheel, + * keyboard and IME path in the canvas is gated on one — and is served a + * thumbnail-grade stream. That must be opt-in, never the default. */ + function renderBare(resizable?: boolean) { + return render(() => ( + + + + )); + } + + it("owns its surface's size when resizable is not mentioned", () => { + renderBare(); + expect(mockSetDisplaySize).toHaveBeenLastCalledWith(800, 600, 120, 120); + }); + + it("stays a passive preview when resizable is false", () => { + renderBare(false); + expect(mockSetDisplaySize).not.toHaveBeenCalled(); + expect(mockRequestResize).not.toHaveBeenCalled(); + }); }); diff --git a/js/ui/src/ConnectionControl.tsx b/js/ui/src/ConnectionControl.tsx deleted file mode 100644 index 8d59c526..00000000 --- a/js/ui/src/ConnectionControl.tsx +++ /dev/null @@ -1,81 +0,0 @@ -/** - * ConnectionControl — one remote's panels, as an overlay of their own. - * - * They used to expand inside the remotes list. That put a unit table and a - * journal inside a row of a list that is itself a dialog, so the thing being - * read was always the narrowest column on the screen, and opening it pushed - * every other remote out of view. - * - * It sits on top of the remotes overlay rather than replacing it: the list is - * where the viewer came from and where closing this returns them, and neither - * layer has to know how the other is dismissed. - */ - -import { onCleanup, onMount } from "solid-js"; -import type { - BlitSession, - BlitSurface, - BlitWorkspace, - ConnectionId, - TerminalPalette, -} from "@blit-sh/core"; -import { ConnectionPanels } from "./ConnectionPanels"; -import { OverlayBackdrop, OverlayHeader, OverlayPanel } from "./Overlay"; -import { claimEscape } from "./overlayStack"; -import { tp } from "./i18n"; - -export function ConnectionControlOverlay(props: { - workspace: BlitWorkspace; - connectionId: ConnectionId; - /** The remote's name, which is also its connection id. Shown in the title. */ - name: string; - palette: TerminalPalette; - fontSize: number; - sessions?: readonly BlitSession[]; - surfaces?: readonly BlitSurface[]; - canListClients: boolean; - canManageExtensions: boolean; - onClose: () => void; -}) { - // Escape closes this, and only this. A listener of our own would not do it: - // the workspace's handler is a capture-phase window listener registered at - // mount, so it sees the key first and closes the remotes overlay underneath — - // one key, two layers dismissed. - onMount(() => onCleanup(claimEscape(() => props.onClose()))); - - return ( - - - - - - - ); -} diff --git a/js/ui/src/ConnectionPanels.tsx b/js/ui/src/ConnectionPanels.tsx index b7a23256..410b4eb3 100644 --- a/js/ui/src/ConnectionPanels.tsx +++ b/js/ui/src/ConnectionPanels.tsx @@ -1,6 +1,12 @@ /** * ConnectionPanels — everything there is to say about ONE remote, as tabs. * + * Hosted by {@link ./ManageTile.tsx}, which is a BSP tile: these are pane + * content, not a dialog. They were a dialog, and the thing that finally settled + * it was Enable in the Session tab — the application it started raised itself, + * an activation closes whatever overlay is up, and the panel dismissed itself + * one second after being used. + * * An expanded remote row used to stack its sections; it now switches between * them, because the set stopped being two short lists. Session and clients are * still short, but a unit table is a thousand rows and a journal page is a @@ -30,19 +36,19 @@ import { ConnectionClients } from "./ConnectionClients"; import { ConnectionSession } from "./ConnectionSession"; import { ExtensionsPanel } from "./ExtensionsPanel"; import { SystemdPanel } from "./SystemdPanel"; +import { + pickedTab, + pickTab, + setShownTab, + TAB_LABELS as LABELS, + type ConnectionTab, +} from "./connectionTab"; import { followChannelNames } from "./channelPresence"; import { SESSION_CHANNEL } from "./session"; import { SYSTEMD_CHANNEL } from "./systemd"; -import { themeFor, ui, uiScale } from "./theme"; - -type Tab = "clients" | "extensions" | "session" | "systemd"; +import { scrollbarStyle, themeFor, ui, uiScale } from "./theme"; -const LABELS: Record = { - clients: "Clients", - extensions: "Extensions", - session: "Session", - systemd: "systemd", -}; +type Tab = ConnectionTab; export function ConnectionPanels(props: { workspace: BlitWorkspace; @@ -62,11 +68,6 @@ export function ConnectionPanels(props: { const [served, setServed] = createSignal>( new Set(), ); - // What the viewer picked, which is not the same as what is shown: an answer - // can land after the click and a tab can vanish under it, so the selection is - // resolved against what exists rather than corrected by an effect that would - // fight the viewer for it. - const [chosen, setChosen] = createSignal(null); // One watch per connection, for both extension channels at once — the answer // is a property of the server's registry, not of either panel. @@ -108,19 +109,47 @@ export function ConnectionPanels(props: { /** The tab actually shown: the pick if it still exists, else the first. */ const tab = (): Tab | null => { const available = tabs(); - const pick = chosen(); - if (pick && available.includes(pick)) return pick; + const picked = pickedTab(props.connectionId); + if (picked && available.includes(picked)) return picked; return available[0] ?? null; }; + // Published for the tile's own card, which has to name this tab while these + // panels are unmounted (`connectionTab.ts`). Only the mounted panels know + // which tabs the server serves, so only they can resolve it. + createEffect(() => setShownTab(props.connectionId, tab())); + return ( - 0}> + 0} + fallback={ + // A pane cannot render nothing the way an overlay section could: the + // viewer asked for this server's panels and is owed the answer that it + // has none. +

+ This server exposes no panels. +

+ } + >
@@ -139,7 +171,7 @@ export function ConnectionPanels(props: { role="tab" data-connection-tab={name} aria-selected={tab() === name} - onClick={() => setChosen(name)} + onClick={() => pickTab(props.connectionId, name)} style={{ ...ui.btn, "border-radius": "0", @@ -161,46 +193,82 @@ export function ConnectionPanels(props: {
- - - - {/* The extensions panel was built as its own overlay, so it carries - its own padding; the wrapper only bounds it. Same for systemd. */} - -
- + + -
- - - - - -
- + {/* The extensions panel was built as its own overlay, so it carries + its own padding; the wrapper only bounds it. Same for systemd. */} + +
+ +
+ + + -
- + + +
+ +
+ +
); diff --git a/js/ui/src/ConnectionSession.tsx b/js/ui/src/ConnectionSession.tsx index b59a3cb9..2a28cf29 100644 --- a/js/ui/src/ConnectionSession.tsx +++ b/js/ui/src/ConnectionSession.tsx @@ -141,6 +141,11 @@ export function ConnectionSession(props: { display: "flex", "flex-direction": "column", "background-color": theme().panelBg, + // Fills the pane region rather than growing past it, so the + // catalog below can be bounded by this box instead of by the + // viewport. + flex: "1 1 auto", + "min-height": "0", }} > } > - - {(app) => ( - -
- + + {(app) => ( + +
- - + - {app.name} - - - - {app.id} - - + {app.name} + + + + {app.id} + + + - - - - {/* Counted from the identity the compositor stamped on - the app's own socket, not from a self-asserted - app_id — which is what makes it worth showing. */} - {app.windows} {app.windows === 1 ? "window" : "windows"} - - {/* Now. Running covers backoff too: a supervisor about + + {/* Counted from the identity the compositor stamped on + the app's own socket, not from a self-asserted + app_id — which is what makes it worth showing. */} + + {app.windows}{" "} + {app.windows === 1 ? "window" : "windows"} + + {/* Now. Running covers backoff too: a supervisor about to retry is something a viewer wants to be able to call off. */} - - {/* Intent, and the way out of the list. Disabling keeps + + {/* Intent, and the way out of the list. Disabling keeps the row -- an application that just failed is worth looking at -- so there has to be something that removes it, or a one-off experiment stays forever. */} - - - -
+ + + +
- {/* Only worth a line when something went wrong: a healthy row + {/* Only worth a line when something went wrong: a healthy row stays one line tall. */} - 0 || app.lastExit !== undefined}> -
- 0}> - {app.failures} failed{" "} - {app.failures === 1 ? "start" : "starts"} - - 0 && app.lastExit !== undefined} + 0 || app.lastExit !== undefined}> +
- {" · "} - - - last exit {app.lastExit} - -
- - - )} - + 0}> + {app.failures} failed{" "} + {app.failures === 1 ? "start" : "starts"} + + 0 && app.lastExit !== undefined} + > + {" · "} + + + last exit {app.lastExit} + +
+ + + )} + + {/* Adding. The whole catalog, scrolling, with the filter narrowing it @@ -375,14 +395,26 @@ export function ConnectionSession(props: { } > - {/* The catalog gets a scroller of its own rather than riding the - overlay's: it is the only unbounded thing here, and letting it - lengthen the panel would scroll the search box — the one - control for a nine-hundred-row list — off the top. */} + {/* The catalog is the only unbounded thing here, so it is the one + thing that scrolls: letting it lengthen the panel instead would + scroll the search box — the one control for a nine-hundred-row + list — off the top. + + Bounded by the pane, not the viewport. It was `42vh`, which in + a dialog capped at 80% of the screen was about right, and in a + pane is a number unrelated to the box it is in: in a short pane + it overflows and the pane scrolls too (two scrollbars for one + list), in a tall one it stops short of the bottom. `flex: 1` + against a parent with `min-height: 0` is the same intent + measured against the right thing. */}
{ + const activateTray = (entry: TrayEntry, gesture: TrayPrimaryGesture) => { if (entry.readOnly) return; - const store = props.workspace.getConnection( - entry.connectionId, - )?.desktopStore; - if (trayPrimaryOpensMenu(entry.item.flags, touch)) { - openTrayMenu(entry); - } else { - store?.activate(entry.item.trayId); + if (gesture === "menu") openTrayMenu(entry); + else if (gesture === "activate") { + props.workspace + .getConnection(entry.connectionId) + ?.desktopStore.activate(entry.item.trayId); } }; @@ -1658,6 +1658,12 @@ export function DesktopChrome(props: { const trayButton = (entry: TrayEntry): JSX.Element => { let primaryPointerType: string | null = null; + // A long press on a touch screen fires `contextmenu` and then a trailing + // `click` on the same press. The press has already opened the menu, so + // letting the click through activated the item as well: the app's window + // came up behind the menu the user was reading, and the repaint that + // followed could take the menu with it. + let openedFromLongPress = false; const icon = imageUrl(entry.item.icon); const title = [ entry.item.tooltipTitle || entry.item.title || entry.item.appId, @@ -1671,16 +1677,23 @@ export function DesktopChrome(props: { disabled={entry.readOnly} onPointerDown={(event) => { primaryPointerType = event.pointerType; + openedFromLongPress = false; }} onPointerCancel={() => { primaryPointerType = null; }} onClick={() => { - const touch = primaryPointerType === "touch"; + const gesture = trayPrimaryGesture( + entry.item.flags, + primaryPointerType, + openedFromLongPress, + ); primaryPointerType = null; - activateTray(entry, touch); + openedFromLongPress = false; + activateTray(entry, gesture); }} onContextMenu={(event) => { + openedFromLongPress = primaryPointerType === "touch"; primaryPointerType = null; event.preventDefault(); openTrayMenu(entry); diff --git a/js/ui/src/ExtensionsPanel.tsx b/js/ui/src/ExtensionsPanel.tsx index f8cfe8a1..4d280c73 100644 --- a/js/ui/src/ExtensionsPanel.tsx +++ b/js/ui/src/ExtensionsPanel.tsx @@ -198,7 +198,10 @@ export function ExtensionsPanel(props: {
diff --git a/js/ui/src/ManageTile.tsx b/js/ui/src/ManageTile.tsx new file mode 100644 index 00000000..8f3f2c77 --- /dev/null +++ b/js/ui/src/ManageTile.tsx @@ -0,0 +1,189 @@ +/** + * ManageTile — one server's panels as pane content, not as a dialog. + * + * The panels used to be a modal stack: the remotes overlay, and on top of it an + * overlay per remote. That made them the least durable thing on the screen. + * Anything that closed an overlay closed these too — and one of the things that + * closes an overlay is a window asking to be raised, which is exactly what + * happens a second after Enable starts an application. So the panel that + * launched the app dismissed itself, and the viewer's next click had to walk + * back in through two dialogs. + * + * A pane has none of that: it is a tile like an editor or a terminal, it can be + * split next to the thing it manages, it survives focus going elsewhere, and it + * is restored by the same hash + tab registry as every other tile. + * + * One tile per connection, from {@link manageAssignment} — the panels hold live + * subscriptions (a client watch pushing a catalog every second, a unit table), + * and two tiles onto one server would run two of each. + */ + +import { createEffect, createSignal, onCleanup, Show } from "solid-js"; +import { createBlitWorkspaceState } from "@blit-sh/solid"; +import type { + BlitSurface, + BlitWorkspace, + ConnectionId, + TerminalPalette, +} from "@blit-sh/core"; +import { ConnectionPanels } from "./ConnectionPanels"; +import { connectionHasClientList } from "./ConnectionClients"; +import { shownTab, TAB_LABELS } from "./connectionTab"; +import { + clearActiveEditor, + setActiveEditorFocused, + type ManageController, +} from "./ide/activeEditor"; +import { + PanelEmpty, + SectionHeading, + StatusPill, + type PanelTone, +} from "./panelKit"; +import { scrollbarStyle, type Theme, type UIScale } from "./theme"; + +/** Connection status → the pill's tone and word. */ +function statusTone(status: string | null): { tone: PanelTone; label: string } { + switch (status) { + case "connected": + return { tone: "ok", label: "connected" }; + case "connecting": + case "authenticating": + return { tone: "warn", label: status }; + case "error": + return { tone: "bad", label: "error" }; + default: + return { tone: "idle", label: status ?? "disconnected" }; + } +} + +export function ManageTile(props: { + workspace: BlitWorkspace; + connectionId: ConnectionId; + theme: Theme; + palette: TerminalPalette; + scale: UIScale; + fontSize: number; + /** The connection is an `.ro` share: the client-control family never + * answers through the forwarder, so the clients tab must not be offered. */ + readOnly?: boolean; + /** Read-only thumbnail. The dock draws no body at all for a manage card — + * its title carries the server and the tab (`tileDisplay`) — so this is the + * floor rather than the case: whatever mounts a preview gets the heading and + * none of the panels, which would otherwise run a per-second client catalog + * and a unit table behind a picture nobody is reading. */ + preview?: boolean; + /** Whether this tile owns workspace focus. BSP keeps every pane mounted, so + * the status bar's identity follows this rather than mounting. */ + focused?: boolean; +}) { + const snapshot = createBlitWorkspaceState(props.workspace); + const connection = () => + snapshot().connections.find((c) => c.id === props.connectionId) ?? null; + const sessions = () => snapshot().sessions; + + // Surfaces, for the client rows' "watching …" labels. Only this connection's: + // the panels filter by connection anyway, so aggregating every server's would + // be work thrown away. + const [surfaces, setSurfaces] = createSignal([]); + createEffect(() => { + // Re-run on reconnect: the store is per BlitConnection, and a connection + // that was absent when this first ran has one now. + void snapshot().connections.length; + const conn = props.workspace.getConnection(props.connectionId); + if (!conn) { + setSurfaces([]); + return; + } + const sync = () => + setSurfaces([...conn.surfaceStore.getSurfaces().values()]); + sync(); + onCleanup(conn.surfaceStore.onChange(sync)); + }); + + // The bar's identity for this pane. Same contract every other tile follows: + // BSP keeps background panes mounted, so ownership tracks focus rather than + // mounting, and a thumbnail never claims it at all. + const controller: ManageController = { + kind: "manage", + connectionId: props.connectionId, + tab: () => { + const name = shownTab(props.connectionId); + return name ? TAB_LABELS[name] : null; + }, + }; + createEffect(() => { + setActiveEditorFocused( + controller, + !props.preview && props.focused !== false, + ); + }); + onCleanup(() => clearActiveEditor(controller)); + + const canListClients = () => { + const conn = connection(); + return ( + !!conn && + connectionHasClientList( + conn, + props.readOnly ? new Set([props.connectionId]) : new Set(), + ) + ); + }; + + return ( +
+ + + + + + + Connect to this remote to manage it. + + } + > + + + +
+ ); +} diff --git a/js/ui/src/RemotesOverlay.tsx b/js/ui/src/RemotesOverlay.tsx index af23a33a..d87f4fa3 100644 --- a/js/ui/src/RemotesOverlay.tsx +++ b/js/ui/src/RemotesOverlay.tsx @@ -1,18 +1,12 @@ import { createSignal, Index, Show } from "solid-js"; import type { BlitConnectionSnapshot, - BlitSession, - BlitSurface, - BlitWorkspace, - ConnectionId, ConnectionStatus, TerminalPalette, } from "@blit-sh/core"; import { OverlayBackdrop, OverlayHeader, OverlayPanel } from "./Overlay"; import { mergeStyle, scrollbarStyle, themeFor, ui, uiScale } from "./theme"; import { createDragReorder, reorderTo } from "./dragReorder"; -import { connectionHasClientList } from "./ConnectionClients"; -import { ConnectionControlOverlay } from "./ConnectionControl"; import { t } from "./i18n"; import type { Remote } from "./storage"; @@ -46,13 +40,13 @@ export function RemotesOverlay(props: { onReorder: (names: string[]) => void; onReconnect?: (name: string) => void; onClose: () => void; - /** Live connections, used to decide which rows can list their clients. - * Omit (with `workspace`) to render the remotes list on its own. */ + /** Live connections, used to decide which rows have anything to manage. + * Omit to render the remotes list on its own. */ connections?: readonly BlitConnectionSnapshot[]; - workspace?: BlitWorkspace; - sessions?: readonly BlitSession[]; - surfaces?: readonly BlitSurface[]; - readOnlyConnections?: ReadonlySet; + /** Open this remote's panels as a pane, and dismiss this dialog. Omit and + * the Manage button stays out of the list — a shell that has nowhere to put + * a tile has nothing to offer here. */ + onManage?: (name: string) => void; }) { const theme = () => themeFor(props.palette); const scale = () => uiScale(props.fontSize); @@ -60,35 +54,17 @@ export function RemotesOverlay(props: { const [name, setName] = createSignal(""); const [uri, setUri] = createSignal(""); const [revealed, setRevealed] = createSignal>(new Set()); - // The remote whose control panel is open, if any. At most one: each open - // panel holds live subscriptions (a CLIENT_WATCH pushing a catalog every - // second, a unit table), and reading two servers side by side is not a - // thing anyone does. - const [controlling, setControlling] = createSignal(null); /** A remote's live connection, if it has one. Remote names *are* connection * ids (App.tsx builds one ConnectionSpec per enabled remote, `id: name`). */ const connectionFor = (remoteName: string) => props.connections?.find((c) => c.id === remoteName); - /** Whether this row can show a client list right now. */ - const canListClients = (remoteName: string) => { - const connection = connectionFor(remoteName); - return ( - !!props.workspace && - !!connection && - connectionHasClientList( - connection, - props.readOnlyConnections ?? new Set(), - ) - ); - }; - - /** Whether this row has anything to control. Every panel needs a live + /** Whether this row has anything to manage. Every panel needs a live * connection to say anything at all, and which of them exist is discovered - * inside the panel rather than here. */ + * inside the pane rather than here. */ const canControl = (remoteName: string) => - !!props.workspace && connectionFor(remoteName)?.status === "connected"; + !!props.onManage && connectionFor(remoteName)?.status === "connected"; /** Any row at all can be controlled — gates the header's explanation of what * the button is, so a shell with nothing connected says nothing. */ @@ -341,9 +317,7 @@ export function RemotesOverlay(props: { ); }; - const clients = () => canListClients(remote().name); const controllable = () => canControl(remote().name); - const open = () => controlling() === remote().name; return ( <> @@ -509,15 +483,16 @@ export function RemotesOverlay(props: { - {/* Clients — a named action rather than a bare - chevron on the name: this replaced a top-level - "Connected clients" entry in the command palette, - and a 1-em glyph is not a discoverable home for - something that used to have its own menu item. - Only offered where the remote could actually - answer; a disconnected row that expanded to "No - clients connected" would be reporting the wrong - thing. */} + {/* Manage — this remote's panels (its applications, + clients, units, extensions) as a pane. A named + action rather than a bare chevron on the name: it + replaced a top-level "Connected clients" entry in + the command palette, and a 1-em glyph is not a + discoverable home for something that used to have + its own menu item. Only offered where the remote + could actually answer; a disconnected row that + opened to "No clients connected" would be + reporting the wrong thing. */} @@ -695,28 +665,6 @@ export function RemotesOverlay(props: { - - {/* On top of this list rather than inside it: a unit table or a journal - page has no business being rendered into a row. Closing it comes back - here, which is where the viewer asked for it. */} - - {(name) => ( - setControlling(null)} - /> - )} - ); } diff --git a/js/ui/src/StatusBar.tsx b/js/ui/src/StatusBar.tsx index 13e53e86..fd90ede6 100644 --- a/js/ui/src/StatusBar.tsx +++ b/js/ui/src/StatusBar.tsx @@ -34,6 +34,7 @@ import { type CommitController, type DiffController, type EditorController, + type ManageController, type PreviewController, } from "./ide/activeEditor"; import { lineWrap, toggleLineWrap } from "./ide/editorPrefs"; @@ -368,6 +369,7 @@ export function StatusBar(props: { + ) : ed.kind === "manage" ? ( + ) : ( - ) : ed.kind === "preview" ? ( - // Nothing to act on: no save, no diff mode, no LSP. + ) : ed.kind === "preview" || ed.kind === "manage" ? ( + // Nothing to act on: no save, no diff mode, no LSP. Everything a + // manage tile can do is a control inside the pane. <> ) : ( @@ -967,6 +972,40 @@ function PathIdentity(props: { ); } +/** Focused manage tile's identity: `dev:manage › Session`. The same two halves + * its dock card has (`tileDisplay`) and the same shape a focused terminal or + * surface puts here — address dim, then the name, which for these panels is + * the tab that is up. */ +function ManageIdentity(props: { + m: ManageController; + label: string | null; +}): JSX.Element { + return ( + <> + + {`${props.label ?? props.m.connectionId}:manage`} + + + {(tab) => ( + <> + {" › "} + + {tab()} + + + )} + + + ); +} + /** Focused commit's identity: repo location, abbreviated oid, subject. */ function CommitIdentity(props: { c: CommitController; diff --git a/js/ui/src/SwitcherOverlay.tsx b/js/ui/src/SwitcherOverlay.tsx index da486e34..e2fb9aa9 100644 --- a/js/ui/src/SwitcherOverlay.tsx +++ b/js/ui/src/SwitcherOverlay.tsx @@ -157,11 +157,13 @@ type RemoteItem = { type TileItem = { type: "tile"; key: string; + /** Dim address half, as a session row has (`dev:manage` before `Session`). */ + prefix: string; title: string; subtitle: string; - /** The tile assignment to restore (editor:/diff:/commit:). */ + /** The tile assignment to restore (editor:/diff:/commit:/manage:). */ assignment: string; - tileKind: "editor" | "diff" | "commit" | "web"; + tileKind: "editor" | "diff" | "commit" | "web" | "manage"; }; type FileItem = { @@ -276,7 +278,7 @@ function PaneGlyph(props: { empty: boolean; fg: string; dimFg: string }) { } function TileGlyph(props: { - kind: "editor" | "diff" | "commit" | "preview" | "web"; + kind: "editor" | "diff" | "commit" | "preview" | "web" | "manage"; fg: string; dimFg: string; }) { @@ -315,6 +317,12 @@ function TileGlyph(props: { + + {/* sliders: a server's own controls */} + + + + {/* framed picture: a rendered file rather than its source */} @@ -648,6 +656,8 @@ function PreviewSurface(props: { + {/* A parked manage tile carries the same + dim address a session row does, so both + are asked for it the same way. */} - - {(item as SessionItem).prefix} - - {" \u203A "} + {(prefix) => ( + <> + + {prefix()} + + + {" \u203A "} + + + )} {item.title} diff --git a/js/ui/src/SystemdLogs.tsx b/js/ui/src/SystemdLogs.tsx index efb1b6e3..70a4e8cd 100644 --- a/js/ui/src/SystemdLogs.tsx +++ b/js/ui/src/SystemdLogs.tsx @@ -264,6 +264,9 @@ export function SystemdLogs(props: { display: "flex", "flex-direction": "column", gap: `${scale().xs}px`, + // Fills the pane region so the journal below is bounded by it. + flex: "1 1 auto", + "min-height": "0", }} >
x.id === sessionId); return !!s && readOnlyConnections().has(s.connectionId); }; + /** The same answer about a whole connection, which is what a manage tile + * needs: read-only shares drop the client-control family, so its clients + * panel must not be offered rather than sit unanswered. */ + const isConnectionReadOnly = (connectionId: string): boolean => + readOnlyConnections().has(connectionId as ConnectionId); const focusedSession = () => { const snap = wsState(); @@ -505,7 +517,8 @@ function WorkspaceScreen(props: { const [surfaces, setSurfaces] = createSignal([]); // Per-surface signature of the fields that drive the thumbnail UI - // (title, appId, width, height). SurfaceStore mutates width/height + // (title, appId, and both size pairs — see surfaceCardSignature). + // SurfaceStore mutates the dimensions // in place on each frame so ref-level diffing never sees dim changes, // and keys by reference so a child component reading // `props.surface.width` won't re-render when the underlying field is @@ -570,7 +583,7 @@ function WorkspaceScreen(props: { for (const s of conn.surfaceStore.getSurfaces().values()) { const key = `${s.connectionId}:${s.surfaceId}`; seenKeys.add(key); - const sig = `${s.title}\0${s.appId}\0${s.width}x${s.height}`; + const sig = surfaceCardSignature(s); if (surfaceSigs.get(key) !== sig) { surfaceSigs.set(key, sig); // Shallow copy: a new ref forces to rebuild this @@ -874,6 +887,9 @@ function WorkspaceScreen(props: { if (typeof assign === "string" && isTileAssignment(assign)) { const t = parseTileAssignment(assign); if (t) { + // A manage tile is a server's panels, not a place in a filesystem: it + // has no root to anchor on, so the last one sticks. + if (t.kind === "manage") return null; if (t.kind === "commit") { const repoPath = t.arg.slice(t.arg.indexOf(":") + 1); return { @@ -1603,15 +1619,43 @@ function WorkspaceScreen(props: { const [focusedSurfaceConnId, setFocusedSurfaceConnId] = createSignal(null); - // What xdg_activation_v1 covered up, newest last (./activationStack.ts). - // Plain `let`: nothing renders it, it only survives between an activation - // and the moment that surface goes away. - let activationStack: string[] = []; - // The main view's occupant, when an activation is what put it there. Only - // its death lowers the stack — a surface the *user* chose replaces the - // covering relationship rather than extending it, so its death clears the - // stack instead of restoring something the user left long ago. - let activatedAssignment: string | null = null; + // Surfaces that asked to come forward (xdg_activation_v1) and were answered + // with a highlight rather than the view — see ./surfaceAttention.ts for why + // an activation must not move anything. + const [attention, setAttention] = createSignal(new Map()); + /** True while `assignment` is lit; what the dock card and the pane read. */ + const hasAttention = (assignment: string) => attention().has(assignment); + // One sweep in flight at a time, aimed at the soonest window to close and + // re-aimed at whatever is left. A timer per request would be a timer per + // *repeat*, and a chatty client sends several a second; a fixed interval + // would leave a later arrival lit past its window, holding off its own next + // pulse for as long as it was late. + let attentionSweep: ReturnType | null = null; + function scheduleAttentionSweep() { + if (attentionSweep != null) return; + const lit = untrack(attention); + if (lit.size === 0) return; + const soonest = Math.min(...lit.values()); + attentionSweep = setTimeout( + () => { + attentionSweep = null; + const next = expireAttention(untrack(attention), Date.now()); + setAttention(next); + scheduleAttentionSweep(); + }, + // A hair past the deadline: expireAttention drops a window only once it + // is strictly over, so landing exactly on it would sweep nothing and + // re-arm for 0ms, in a loop. + Math.max(16, soonest - Date.now() + 16), + ); + } + function flashAttention(assignment: string) { + setAttention((prev) => armAttention(prev, assignment, Date.now())); + scheduleAttentionSweep(); + } + onCleanup(() => { + if (attentionSweep != null) clearTimeout(attentionSweep); + }); /** Set or clear the focused surface, always keeping the connectionId * in sync so the BSP view uses the correct connection. @@ -2101,7 +2145,7 @@ function WorkspaceScreen(props: { clearTimeout(clearFocusedTimer); clearFocusedTimer = null; } - lowerFocusedSurface(fid, fConnId); + focusSurfaceById(null); } else if (!exists) { if (!clearFocusedTimer) { clearFocusedTimer = setTimeout(() => { @@ -2112,7 +2156,7 @@ function WorkspaceScreen(props: { s.surfaceId === fid && (fConnId == null || s.connectionId === fConnId), ); - if (stillGone) lowerFocusedSurface(fid, fConnId); + if (stillGone) focusSurfaceById(null); }, 2000); } } else if (clearFocusedTimer) { @@ -2122,8 +2166,18 @@ function WorkspaceScreen(props: { }); const offScreenSurfaces = createMemo(() => { - const fid = focusedSurfaceId(); - const fConnId = focusedSurfaceConnId(); + // A tile covers the main view (it is drawn ahead of the focused surface), + // so the surface underneath is off-screen and belongs in the panel — the + // same rule the sessions memo below applies to a displaced terminal. + // Without this, tapping a tile's dock card hid the surface it covered from + // everywhere at once: the tile is on top of it, and this filter dropped it + // from the panel because focusedSurfaceId still named it. It came back + // only by closing the tile. The slot is deliberately still *set* — that is + // what brings the surface back when the tile closes — so what changes here + // is only whether it is also offered as a card. + const covered = activeTile() != null; + const fid = covered ? null : focusedSurfaceId(); + const fConnId = covered ? null : focusedSurfaceConnId(); // Collect surface keys assigned to BSP panes. const al = activeLayout(); const la = layoutAssignments(); @@ -3264,6 +3318,41 @@ function WorkspaceScreen(props: { }); onCleanup(() => document.getElementById("blit-scrollbars")?.remove()); + // The highlight an xdg_activation_v1 request buys instead of the view: red, + // fading out over the window. One colour and one direction — a two-colour + // bounce read as a state change rather than a nudge, and it had to be + // explained. Nothing here moves, so there is no reduced-motion variant to + // offer either. Global and themed like the scrollbars above, because the same + // fade has to land on two very different things — a dock card's header bar + // and a pane-sized ring — and keyframes cannot be inline styles. + createEffect(() => { + const t = theme(); + const id = "blit-attention"; + let el = document.getElementById(id) as HTMLStyleElement | null; + if (!el) { + el = document.createElement("style"); + el.id = id; + document.head.appendChild(el); + } + el.textContent = ` + @keyframes blit-attention-fill { + 0% { background-color: ${t.errorText}; } + 100% { background-color: transparent; } + } + @keyframes blit-attention-ring { + 0% { border-color: ${t.errorText}; } + 100% { border-color: transparent; } + } + [data-blit-attention="fill"] { + animation: blit-attention-fill ${ATTENTION_MS}ms ease-out 1; + } + [data-blit-attention="ring"] { + animation: blit-attention-ring ${ATTENTION_MS}ms ease-out 1; + } + `; + }); + onCleanup(() => document.getElementById("blit-attention")?.remove()); + onMount(() => { document.documentElement.style.fontFamily = "system-ui, sans-serif"; }); @@ -3730,28 +3819,26 @@ function WorkspaceScreen(props: { /** * A Wayland client asked for its own toplevel (xdg_activation_v1 — an - * Electron app reacting to a notification click). It gets the same treatment - * as picking the surface in the switcher, plus a record of what it covered. + * Electron app reacting to a notification click). It is answered with a + * highlight where the surface already is, and nothing else: the view is the + * user's, and an app that wants it can only ask to be looked at. * - * BSP pushes nothing: `focusSurface` either focuses the pane the surface is - * already in or hands it the focused pane, and a pane it displaces keeps its - * occupant in the dock where the user can see it. The non-BSP main view is - * one slot, so without the stack the previous occupant is simply gone once - * the activated surface closes. + * Raising instead is what made the dock unusable next to a talkative client. + * Tokens are cheap and their delivery unacknowledged, so a client repeats the + * request several times a second, and each repeat landed after whatever the + * user had just picked — their choice appearing for an instant and being + * dragged back off, with repeated clicking working only when one fell in a + * gap. Under a layout it was worse: each repeat re-focused a pane out from + * under them. See ./surfaceAttention.ts. */ function activateSurface(surfaceId: number, connectionId: ConnectionId) { - const target = surfaceAssignment(connectionId, surfaceId); - // Already on top: nothing to raise, and nothing to remember. Clients - // repeat the request (a token is cheap and its delivery unacknowledged), - // so without this a talkative app would re-run focusSurface — closing an - // overlay the user just opened — several times a second. + // Already on top: the user is looking straight at it, so lighting it up + // would be noise rather than news. // // "On top" is a different slot in each mode: focusedSurfaceId is the - // non-BSP main view, which focusSurface nulls under a layout, so testing - // only that left this guard dead in BSP — where the repeats are worse than - // a closed overlay, since each one re-focuses the pane out from under the - // user. In BSP the equivalent question is whether the surface already - // occupies the focused pane. + // non-BSP main view, which is left null under a layout, so testing only + // that would leave this dead in BSP. There the equivalent question is + // whether the surface already occupies the focused pane. if (inBsp()) { const focused = bspFocusedSurface(); if ( @@ -3766,70 +3853,7 @@ function WorkspaceScreen(props: { ) { return; } - if (inBsp()) { - activationStack = []; - activatedAssignment = null; - } else { - activationStack = pushActivation( - activationStack, - focusedAssignment(), - target, - ); - activatedAssignment = target; - } - focusSurface(surfaceId, connectionId); - } - - /** Show a stacked entry again. Deliberately not `focusAssignment`: this runs - * because a window closed, not because the user asked for anything, so it - * must not close an overlay they have open. */ - function restoreMainView(assignment: string) { - const surface = parseSurfaceAssignment(assignment); - if (surface) { - setActiveTile(null); - focusSurfaceById(surface.surfaceId, surface.connectionId as ConnectionId); - return; - } - if (isTileAssignment(assignment) || isWebAssignment(assignment)) { - focusSurfaceById(null); - setActiveTile(assignment); - return; - } - focusSessionFromUi(assignment as SessionId); - } - - /** - * The focused surface is gone. If an activation is what put it on screen, - * reveal what it covered; otherwise clear the slot exactly as before, and - * drop the stack — the user moved on from that chain, so its entries would - * only resurface somewhere they no longer belong. - */ - function lowerFocusedSurface( - surfaceId: number, - connectionId: ConnectionId | null, - ) { - const dying = - connectionId != null ? surfaceAssignment(connectionId, surfaceId) : null; - if (dying != null && dying === activatedAssignment) { - // cycleRing is every open terminal, surface and tab, so one lookup - // covers all four things a stack entry can name. - const open = new Set(cycleRing()); - const { restore, stack } = popActivation(activationStack, (a) => - open.has(a), - ); - activationStack = stack; - if (restore != null) { - // A restored entry is still part of the activation chain: it only sits - // there because an activation covered it, so its own death lowers the - // stack again. - activatedAssignment = restore; - restoreMainView(restore); - return; - } - } - activationStack = []; - activatedAssignment = null; - focusSurfaceById(null); + flashAttention(surfaceAssignment(connectionId, surfaceId)); } let termHandle: { rows: number; cols: number; focus: () => void } | null = @@ -4688,6 +4712,7 @@ function WorkspaceScreen(props: { fontFamily={resolvedFontWithFallback()} fontSize={fontSize()} onOpenTile={openTile} + isConnectionReadOnly={isConnectionReadOnly} />
)} @@ -4718,6 +4743,7 @@ function WorkspaceScreen(props: { onLayoutChange={setBspLayout} connectionId={activeConnectionId()} isSessionReadOnly={isSessionReadOnly} + isConnectionReadOnly={isConnectionReadOnly} connectionLabels={connectionLabels()} palette={palette()} fontFamily={resolvedFontWithFallback()} @@ -4730,6 +4756,7 @@ function WorkspaceScreen(props: { liveSurfaceKeys={surfaces().map( (s) => `${s.connectionId}:${s.surfaceId}`, )} + hasAttention={hasAttention} manageVisibility={overlay() !== "expose"} extraVisibleSessions={ previewPanelVisible() @@ -4795,6 +4822,7 @@ function WorkspaceScreen(props: { surfaces={offScreenSurfaces()} focusedSurfaceId={focusedSurfaceId()} focusedSurfaceConnId={focusedSurfaceConnId()} + hasAttention={hasAttention} connectionId={activeConnectionId()} connectionLabels={connectionLabels()} theme={theme()} @@ -4817,7 +4845,9 @@ function WorkspaceScreen(props: { backgroundEditors={ {(assignment, index) => { - const d = tileDisplay(assignment); + // Re-read, not read once: a manage tile's title carries the + // tab its panels are on, which changes under the card. + const d = () => tileDisplay(assignment); const web = parseWebAssignment(assignment); return ( // The same card parked terminals and surfaces get: @@ -4851,9 +4881,19 @@ function WorkspaceScreen(props: { "font-size": `${chromeScale().sm}px`, }} > - {d.title} + {/* Address dim, then the name — the same shape + the terminal and surface cards below use, so + a column of parked things reads as one list + rather than three conventions. */} + + + {d().prefix} + + {" \u203A "} + + {d().title} - + - {d.subtitle} + {d().subtitle} @@ -4876,7 +4916,19 @@ function WorkspaceScreen(props: { // mounted preview editor holds an fs sync and a web // preview holds an iframe, so both are budgeted // (LIVE_DOCK_PREVIEWS). - + // + // A manage tile has no picture worth taking: its + // panels are lists of text at a size nobody can read, + // and mounting them to draw that would run a client + // catalog every second behind the card. Its title + // says which server and which tab, which is the whole + // of what the card is picked by. +
} @@ -5177,10 +5230,12 @@ function WorkspaceScreen(props: { onReconnect={(name) => workspace.reconnectConnection(name)} onClose={closeOverlay} connections={allConnections()} - workspace={workspace} - sessions={wsState().sessions} - surfaces={surfaces()} - readOnlyConnections={readOnlyConnections()} + onManage={(name) => { + // The panels are a tile, so the dialog that asked for them is + // in the way once they exist. + closeOverlay(); + openTile(manageAssignment(name)); + }} /> )} @@ -5453,6 +5508,8 @@ function PreviewPanel(props: { surfaces: BlitSurface[]; focusedSurfaceId: number | null; focusedSurfaceConnId: ConnectionId | null; + /** Is this pane assignment currently lit by an activation? */ + hasAttention: (assignment: string) => boolean; connectionId: string; connectionLabels?: Map; theme: Theme; @@ -5678,6 +5735,9 @@ function PreviewPanel(props: { s().surfaceId === props.focusedSurfaceId && s().connectionId === props.focusedSurfaceConnId } + attention={props.hasAttention( + surfaceAssignment(s().connectionId, s().surfaceId), + )} isMobileTouch={props.isMobileTouch} onFocus={() => props.onFocusSurface(s().connectionId, s().surfaceId) @@ -5715,6 +5775,8 @@ function Thumbnail(props: { closeTitle: string; /** Extra header-bar background (e.g. for focused highlight). */ headerBg?: string; + /** Pulse the header: this card's content asked to come forward. */ + attention?: boolean; /** Inline elements rendered inside the header button. */ header: () => any; /** Body content (terminal preview, surface view, etc.). */ @@ -5818,6 +5880,11 @@ function Thumbnail(props: { >