From 36fb8bfa9a879abae29dace85ab3e643d5f51cd8 Mon Sep 17 00:00:00 2001 From: Mollusk Date: Fri, 26 Jun 2026 17:37:49 -0400 Subject: [PATCH] =?UTF-8?q?fix(screenshare):=20address=20Codex=20A23=20aud?= =?UTF-8?q?it=20P3s=20=E2=80=94=20picker=20state=20machine,=20enum=20parit?= =?UTF-8?q?y,=20pactl=20timeout?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Triage of Codex's read-only A23 audit (a23-audit-2026-06-26.md). The P2 (pixelpass --app best-effort fallback) is cross-repo and deferred to a design decision; these are the three actionable peerspeak-side P3s: - P3-1: guard the share-startup window. New `share_starting` flag blocks reopening the picker (and re-firing StartScreenShare) between ConfirmShareScreen and the core's ScreenShareStarted; cleared on Started/Stopped/Error so a failed spawn (surfaced as Error, not Stopped) can't wedge it. +2 state-machine tests. - P3-2: parse_audio_apps now runs each name through sanitize_app_name, so the picker never offers a name that host_args would later silently drop (which would revert the share to whole-desktop audio = the A23 echo, with no signal). +1 test. - P3-3: list_audio_apps wraps pactl in a 2s timeout so a wedged enumeration can't stall the core command loop (mute/deafen/leave/stop) while the picker opens. 433 lib tests (+3), clippy --all-targets clean. Co-Authored-By: Claude Opus 4.8 --- src/app/mod.rs | 102 +++++++++++++++++++++++++++++++++++++---- src/screenshare/mod.rs | 45 ++++++++++++++---- 2 files changed, 129 insertions(+), 18 deletions(-) diff --git a/src/app/mod.rs b/src/app/mod.rs index 4de68e1..bb37374 100644 --- a/src/app/mod.rs +++ b/src/app/mod.rs @@ -583,6 +583,11 @@ pub struct AppState { /// The picker's current selection: `Some(name)` = capture that app's audio, /// `None` = "All system audio" (whole desktop; may echo the call). share_audio_selection: Option, + /// A share start is in flight: `ConfirmShareScreen` was sent but the core + /// hasn't yet replied with `ScreenShareStarted`/an error. Blocks reopening + /// the picker (and re-confirming) during that startup window. Cleared on + /// `ScreenShareStarted`, `ScreenShareStopped`, or any `Error`. + share_starting: bool, /// Whether the Chat drawer is open (drawer layout only). drawer_chat_open: bool, /// Raw mic level (normalized RMS, `0.0..=1.0`) for the settings meter. @@ -655,6 +660,7 @@ impl AppState { self.share_picker_open = false; self.share_audio_apps.clear(); self.share_audio_selection = None; + self.share_starting = false; } fn custom_sound_path(&self, sound: Sound) -> &str { @@ -779,6 +785,7 @@ impl Default for AppState { share_picker_open: false, share_audio_apps: Vec::new(), share_audio_selection: None, + share_starting: false, drawer_chat_open: false, mic_level: 0.0, mic_test_active: false, @@ -1162,12 +1169,14 @@ fn update(state: &mut AppState, message: AppMessage) -> Task { AppMessage::ToggleScreenShare => { if state.self_sharing { let _ = state.controller.send(CoreCommand::StopScreenShare); - } else { + } else if !state.share_starting { // Open the audio picker instead of sharing immediately, so the // user chooses which app's audio to capture rather than the whole // desktop (which echoes the call back to viewers, A23). Default // selection is "All system audio" (None). Kick off a fresh // enumeration so the list reflects what's playing right now. + // Suppressed while a start is already in flight (`share_starting`) + // so the picker can't be reopened during the startup window. state.share_picker_open = true; state.share_audio_selection = None; state.share_audio_apps.clear(); @@ -1181,12 +1190,18 @@ fn update(state: &mut AppState, message: AppMessage) -> Task { state.share_audio_selection = app; } AppMessage::ConfirmShareScreen => { - state.share_picker_open = false; - let audio_app = state.share_audio_selection.clone(); - let _ = state - .controller - .send(CoreCommand::StartScreenShare { audio_app }); - state.status_message = "Starting screen share…".to_string(); + // Only a confirm from an open picker starts a share; a stray confirm + // (or one arriving while a start is already in flight) is ignored, so + // we can't double-send StartScreenShare. + if state.share_picker_open && !state.share_starting { + state.share_picker_open = false; + state.share_starting = true; + let audio_app = state.share_audio_selection.clone(); + let _ = state + .controller + .send(CoreCommand::StartScreenShare { audio_app }); + state.status_message = "Starting screen share…".to_string(); + } } AppMessage::WatchShare(ticket) => { let _ = state.controller.send(CoreCommand::ViewShare(ticket)); @@ -1359,10 +1374,14 @@ fn update(state: &mut AppState, message: AppMessage) -> Task { } UiEvent::ScreenShareStarted => { state.self_sharing = true; + state.share_starting = false; + // Defensive: ensure no picker lingers across a successful start. + state.share_picker_open = false; state.status_message = "Sharing your screen".to_string(); } UiEvent::ScreenShareStopped => { state.self_sharing = false; + state.share_starting = false; state.status_message = "Screen share stopped".to_string(); } UiEvent::IdentityStatus { node_id, persisted, error } => { @@ -1407,6 +1426,10 @@ fn update(state: &mut AppState, message: AppMessage) -> Task { } } UiEvent::Error(err) => { + // A failed share start (spawn error) surfaces here, not via + // ScreenShareStopped, so clear the in-flight flag to let the + // user retry instead of being wedged. + state.share_starting = false; state.status_message = format!("Error: {}", err); } } @@ -6128,8 +6151,9 @@ impl Program for Icon { mod tests { use super::{ attachment_default_name, format_duration, initial_window_position, reconnect_attempt_chime, - reconnected_chime, set_peer_gate_config, set_peer_volume_config, AppConfig, AppState, - AttachmentCache, AttachmentState, ChatEntry, GateMeter, METER_MAX, + reconnected_chime, set_peer_gate_config, set_peer_volume_config, update, AppConfig, + AppMessage, AppState, AttachmentCache, AttachmentState, ChatEntry, GateMeter, METER_MAX, + UiEvent, }; use iroh::SecretKey; @@ -6292,6 +6316,7 @@ mod tests { state.share_picker_open = true; state.share_audio_apps = vec!["Firefox".to_string()]; state.share_audio_selection = Some("Firefox".to_string()); + state.share_starting = true; state.clip_status.lock().unwrap().playing_id = Some(attachment_id); state.reset_room_state(); @@ -6315,6 +6340,7 @@ mod tests { assert!(!state.share_picker_open); assert!(state.share_audio_apps.is_empty()); assert!(state.share_audio_selection.is_none()); + assert!(!state.share_starting); for _ in 0..50 { if crate::audio::clip_player::status_snapshot(&state.clip_status).playing_id.is_none() { @@ -6325,6 +6351,64 @@ mod tests { panic!("clip player did not stop during room reset"); } + #[test] + fn share_picker_startup_window_is_guarded() { + // P3-1: between confirming the picker and the core's ScreenShareStarted, + // self_sharing is still false. The picker must not be reopenable in that + // window, and a stray confirm must not re-fire StartScreenShare. + // Picker open, user confirms a selection. + let mut state = AppState { + share_picker_open: true, + share_audio_selection: Some("mpv".to_string()), + ..Default::default() + }; + let _ = update(&mut state, AppMessage::ConfirmShareScreen); + assert!(state.share_starting, "confirm should mark a start in flight"); + assert!(!state.share_picker_open, "confirm should close the picker"); + assert!(!state.self_sharing, "core hasn't acked the start yet"); + + // Clicking Share again during startup must NOT reopen the picker. + let _ = update(&mut state, AppMessage::ToggleScreenShare); + assert!(!state.share_picker_open, "picker must stay closed while starting"); + assert!(state.share_starting); + + // A stray confirm during startup is ignored (no double-start). + let _ = update(&mut state, AppMessage::ConfirmShareScreen); + assert!(state.share_starting); + + // Core acks: flag clears, sharing begins, no picker lingers. + let _ = update( + &mut state, + AppMessage::UiEventReceived(UiEvent::ScreenShareStarted), + ); + assert!(!state.share_starting); + assert!(state.self_sharing); + assert!(!state.share_picker_open); + } + + #[test] + fn share_start_failure_clears_in_flight_flag() { + // A failed spawn surfaces as UiEvent::Error (not ScreenShareStopped); the + // in-flight flag must still clear so the user can retry. + let mut state = AppState { + share_picker_open: true, + ..Default::default() + }; + let _ = update(&mut state, AppMessage::ConfirmShareScreen); + assert!(state.share_starting); + + let _ = update( + &mut state, + AppMessage::UiEventReceived(UiEvent::Error("boom".into())), + ); + assert!(!state.share_starting, "error must un-wedge the start flag"); + assert!(!state.self_sharing); + + // And now the picker can be opened again. + let _ = update(&mut state, AppMessage::ToggleScreenShare); + assert!(state.share_picker_open); + } + #[test] fn peer_gate_persists_when_on_and_clears_when_off() { let mut config = AppConfig::default(); diff --git a/src/screenshare/mod.rs b/src/screenshare/mod.rs index fb40ac2..7845e1a 100644 --- a/src/screenshare/mod.rs +++ b/src/screenshare/mod.rs @@ -145,28 +145,38 @@ pub fn sanitize_app_name(name: &str) -> Option { ok.then(|| name.to_string()) } +/// Hard cap on how long enumeration waits for `pactl`. It runs inline on the core +/// command loop (the picker awaits it before opening), so a wedged/slow `pactl` +/// must not stall mute/deafen/leave/stop. On timeout we treat it like any other +/// failure: empty list → "All system audio" only. +const LIST_APPS_TIMEOUT: Duration = Duration::from_secs(2); + /// Enumerate the apps currently sending audio to a sink, deduplicated by /// `application.name`. Mirrors how pixelpass itself builds its interactive /// picker (`pactl -f json list sink-inputs`), so the names we return are exactly /// the ones `--app` matches against. Returns an empty list on any error (pactl -/// missing, non-PipeWire host, nothing playing) — a normal, handled state that -/// leaves the picker showing only "All system audio". +/// missing, non-PipeWire host, nothing playing, or [`LIST_APPS_TIMEOUT`] elapsed) +/// — a normal, handled state that leaves the picker showing only "All system +/// audio". pub async fn list_audio_apps() -> Vec { - let output = Command::new("pactl") + let run = Command::new("pactl") .args(["-f", "json", "list", "sink-inputs"]) .stdin(Stdio::null()) .stdout(Stdio::piped()) .stderr(Stdio::null()) - .output() - .await; - match output { - Ok(o) if o.status.success() => parse_audio_apps(&o.stdout), + .output(); + match tokio::time::timeout(LIST_APPS_TIMEOUT, run).await { + Ok(Ok(o)) if o.status.success() => parse_audio_apps(&o.stdout), _ => Vec::new(), } } /// Parse `pactl -f json list sink-inputs` stdout into a sorted, deduplicated list /// of `application.name`s. Pure: no I/O. Unparseable input yields an empty list. +/// Each name is passed through [`sanitize_app_name`] so the picker only ever +/// offers names that will actually survive [`host_args`]; otherwise a name that +/// parses here but fails sanitization later would be selectable yet silently +/// drop the `--app` flag and revert the share to whole-desktop audio (A23 echo). pub fn parse_audio_apps(stdout: &[u8]) -> Vec { let Ok(entries) = serde_json::from_slice::>(stdout) else { return Vec::new(); @@ -174,8 +184,7 @@ pub fn parse_audio_apps(stdout: &[u8]) -> Vec { let mut names: Vec = entries .into_iter() .filter_map(|e| e.properties.application_name) - .map(|n| n.trim().to_string()) - .filter(|n| !n.is_empty()) + .filter_map(|n| sanitize_app_name(&n)) .collect(); names.sort_unstable(); names.dedup(); @@ -532,6 +541,24 @@ mod tests { assert_eq!(parse_audio_apps(b"[]"), Vec::::new()); } + #[test] + fn parse_audio_apps_drops_names_host_args_would_reject() { + // Names that parse from pactl but fail `sanitize_app_name` (control chars, + // overlong) must NOT be offered in the picker — otherwise the user could + // pick one, `host_args` would silently drop `--app`, and the share would + // revert to whole-desktop audio (A23 echo) with no signal. The valid name + // survives; the control-char and overlong ones are filtered out. + let overlong = "x".repeat(MAX_APP_NAME_LEN + 1); + let stdout = format!( + r#"[ + {{"index":1,"properties":{{"application.name":"mpv"}}}}, + {{"index":2,"properties":{{"application.name":"bad\nname"}}}}, + {{"index":3,"properties":{{"application.name":"{overlong}"}}}} + ]"# + ); + assert_eq!(parse_audio_apps(stdout.as_bytes()), vec!["mpv".to_string()]); + } + #[test] fn sanitize_ticket_accepts_pixelpass_endpoint_ticket_shape() { let ticket = "endpointaabwxjexzensznfvuudiapn5tyzws3angd2merarm";