From ac672ba6fa42fa0f7e78645a13779b2caf51a64f Mon Sep 17 00:00:00 2001 From: Oz Date: Tue, 11 Aug 2026 19:26:16 +0000 Subject: [PATCH 1/2] Expose shared session inactivity settings in Features > Session Surfaces the three existing shared-session inactivity durations (revoke edit access, warn, end session) as user-configurable settings below the existing confirm-close-shared-session toggle. Values are edited in minutes and are validated so revoke <= warn <= end always holds (editing one field clamps it against the other two's current values). No inactivity timeout behavior, defaults, or the sharer inactivity ladder logic in view_impl.rs change. The three settings move from private to public (private: false) with a toml_path and description, matching the convention used by other user-facing settings (e.g. ShouldConfirmCloseSession), so they persist via the normal settings pipeline and take effect on the running session. APP-5313 Co-Authored-By: Warp Agent --- app/src/settings_view/features/mod.rs | 3 + .../features/shared_session_inactivity.rs | 454 ++++++++++++++++++ app/src/settings_view/features_page.rs | 26 + app/src/terminal/shared_session/settings.rs | 12 +- 4 files changed, 492 insertions(+), 3 deletions(-) create mode 100644 app/src/settings_view/features/shared_session_inactivity.rs diff --git a/app/src/settings_view/features/mod.rs b/app/src/settings_view/features/mod.rs index 7c2540741e0..091431d62a4 100644 --- a/app/src/settings_view/features/mod.rs +++ b/app/src/settings_view/features/mod.rs @@ -1,3 +1,6 @@ +pub mod shared_session_inactivity; +pub use shared_session_inactivity::SharedSessionInactivityView; + pub mod undo_close; pub use undo_close::UndoCloseView; diff --git a/app/src/settings_view/features/shared_session_inactivity.rs b/app/src/settings_view/features/shared_session_inactivity.rs new file mode 100644 index 00000000000..ebc97965b53 --- /dev/null +++ b/app/src/settings_view/features/shared_session_inactivity.rs @@ -0,0 +1,454 @@ +use std::cell::RefCell; +use std::collections::HashMap; +use std::time::Duration; + +use settings::Setting; +use warp_errors::report_if_error; +use warpui::elements::{ + Container, CrossAxisAlignment, Flex, MouseStateHandle, ParentElement, Text, +}; +use warpui::ui_components::components::{Coords, UiComponent, UiComponentStyles}; +use warpui::{ + AppContext, Element, Entity, SingletonEntity, TypedActionView, View, ViewContext, ViewHandle, +}; + +use crate::appearance::Appearance; +use crate::editor::{self, EditorView, SingleLineEditorOptions, TextOptions}; +use crate::settings_view::settings_page::{LocalOnlyIconState, ToggleState, render_body_item}; +use crate::terminal::shared_session::settings::{ + InactivityPeriodBeforeEndingSession, InactivityPeriodBeforeRevokingRoles, + InactivityPeriodBeforeWarning, SharedSessionSettings, SharedSessionSettingsChangedEvent, +}; + +/// Minimum allowed value for any of the inactivity durations, in minutes. +const MIN_MINUTES: u64 = 1; + +#[derive(Debug, Clone, Copy)] +#[allow(clippy::enum_variant_names)] +pub enum Action { + /// The "revoke edit access after" duration editor was committed. + RevokeEditAccessAfterChanged, + /// The "warn before ending session after" duration editor was committed. + WarningAfterChanged, + /// The "end session after" duration editor was committed. + EndSessionAfterChanged, +} + +/// A view containing settings that control how long a shared session can sit +/// idle before the sharer's edit access ladder kicks in: edit access is +/// revoked, then a warning is shown, then the session ends. This view only +/// exposes those existing durations for editing; it does not change the +/// underlying inactivity behavior. +pub struct SharedSessionInactivityView { + revoke_edit_access_editor: ViewHandle, + warning_editor: ViewHandle, + end_session_editor: ViewHandle, + is_revoke_edit_access_valid: bool, + is_warning_valid: bool, + is_end_session_valid: bool, + local_only_icon_states: RefCell>, +} + +impl SharedSessionInactivityView { + pub fn new(ctx: &mut ViewContext) -> Self { + let editor_options = SingleLineEditorOptions { + text: TextOptions::ui_font_size(Appearance::as_ref(ctx)), + ..Default::default() + }; + + let revoke_edit_access_editor = + ctx.add_typed_action_view(|ctx| EditorView::single_line(editor_options.clone(), ctx)); + let warning_editor = + ctx.add_typed_action_view(|ctx| EditorView::single_line(editor_options.clone(), ctx)); + let end_session_editor = + ctx.add_typed_action_view(|ctx| EditorView::single_line(editor_options.clone(), ctx)); + + ctx.subscribe_to_model( + &SharedSessionSettings::handle(ctx), + |me, settings, event, ctx| { + match event { + SharedSessionSettingsChangedEvent::InactivityPeriodBeforeRevokingRoles { + .. + } => { + me.revoke_edit_access_editor.update(ctx, |editor, ctx| { + let minutes = Self::minutes( + *settings.as_ref(ctx).inactivity_period_before_revoking_roles, + ); + editor.set_buffer_text(&minutes.to_string(), ctx); + }); + } + SharedSessionSettingsChangedEvent::InactivityPeriodBeforeWarning { .. } => { + me.warning_editor.update(ctx, |editor, ctx| { + let minutes = Self::minutes( + *settings.as_ref(ctx).inactivity_period_before_warning, + ); + editor.set_buffer_text(&minutes.to_string(), ctx); + }); + } + SharedSessionSettingsChangedEvent::InactivityPeriodBeforeEndingSession { + .. + } => { + me.end_session_editor.update(ctx, |editor, ctx| { + let minutes = Self::minutes( + *settings.as_ref(ctx).inactivity_period_before_ending_session, + ); + editor.set_buffer_text(&minutes.to_string(), ctx); + }); + } + _ => {} + } + ctx.notify(); + }, + ); + + ctx.subscribe_to_view(&revoke_edit_access_editor, move |me, _, event, ctx| { + me.handle_revoke_edit_access_editor_event(event, ctx); + }); + ctx.subscribe_to_view(&warning_editor, move |me, _, event, ctx| { + me.handle_warning_editor_event(event, ctx); + }); + ctx.subscribe_to_view(&end_session_editor, move |me, _, event, ctx| { + me.handle_end_session_editor_event(event, ctx); + }); + + let settings = SharedSessionSettings::as_ref(ctx); + let revoke_minutes = Self::minutes(*settings.inactivity_period_before_revoking_roles); + let warning_minutes = Self::minutes(*settings.inactivity_period_before_warning); + let end_minutes = Self::minutes(*settings.inactivity_period_before_ending_session); + + revoke_edit_access_editor.update(ctx, |editor, ctx| { + editor.set_buffer_text(&revoke_minutes.to_string(), ctx); + }); + warning_editor.update(ctx, |editor, ctx| { + editor.set_buffer_text(&warning_minutes.to_string(), ctx); + }); + end_session_editor.update(ctx, |editor, ctx| { + editor.set_buffer_text(&end_minutes.to_string(), ctx); + }); + + Self { + revoke_edit_access_editor, + warning_editor, + end_session_editor, + is_revoke_edit_access_valid: true, + is_warning_valid: true, + is_end_session_valid: true, + local_only_icon_states: Default::default(), + } + } + + /// Parses user-entered text into a positive number of minutes, returning + /// `None` if the text isn't a valid, positive integer. + fn parse_minutes(text: &str) -> Option { + text.trim() + .parse::() + .ok() + .filter(|&minutes| minutes >= MIN_MINUTES) + } + + /// Converts a duration to whole minutes for display, rounding up so that + /// a duration is never displayed as zero minutes. + fn minutes(duration: Duration) -> u64 { + duration.as_secs().div_ceil(60).max(MIN_MINUTES) + } + + fn handle_revoke_edit_access_editor_event( + &mut self, + event: &editor::Event, + ctx: &mut ViewContext, + ) { + use editor::Event; + match event { + Event::Edited(_) => { + let text = self.revoke_edit_access_editor.as_ref(ctx).buffer_text(ctx); + let is_valid = Self::parse_minutes(&text).is_some(); + if is_valid != self.is_revoke_edit_access_valid { + self.is_revoke_edit_access_valid = is_valid; + ctx.notify(); + } + } + Event::Blurred | Event::Enter => { + self.handle_action(&Action::RevokeEditAccessAfterChanged, ctx); + } + _ => (), + } + } + + fn handle_warning_editor_event(&mut self, event: &editor::Event, ctx: &mut ViewContext) { + use editor::Event; + match event { + Event::Edited(_) => { + let text = self.warning_editor.as_ref(ctx).buffer_text(ctx); + let is_valid = Self::parse_minutes(&text).is_some(); + if is_valid != self.is_warning_valid { + self.is_warning_valid = is_valid; + ctx.notify(); + } + } + Event::Blurred | Event::Enter => { + self.handle_action(&Action::WarningAfterChanged, ctx); + } + _ => (), + } + } + + fn handle_end_session_editor_event( + &mut self, + event: &editor::Event, + ctx: &mut ViewContext, + ) { + use editor::Event; + match event { + Event::Edited(_) => { + let text = self.end_session_editor.as_ref(ctx).buffer_text(ctx); + let is_valid = Self::parse_minutes(&text).is_some(); + if is_valid != self.is_end_session_valid { + self.is_end_session_valid = is_valid; + ctx.notify(); + } + } + Event::Blurred | Event::Enter => { + self.handle_action(&Action::EndSessionAfterChanged, ctx); + } + _ => (), + } + } + + /// Commits a new value for the "revoke edit access" duration, clamping it + /// so the revoke -> warn -> end ordering is preserved. The other two + /// durations are never modified as a result of this edit. + fn commit_revoke_edit_access(&mut self, ctx: &mut ViewContext) { + let text = self.revoke_edit_access_editor.as_ref(ctx).buffer_text(ctx); + let Some(minutes) = Self::parse_minutes(&text) else { + self.is_revoke_edit_access_valid = false; + ctx.notify(); + return; + }; + self.is_revoke_edit_access_valid = true; + + let settings = SharedSessionSettings::as_ref(ctx); + let warning_minutes = Self::minutes(*settings.inactivity_period_before_warning); + let end_minutes = Self::minutes(*settings.inactivity_period_before_ending_session); + let clamped = minutes.min(warning_minutes).min(end_minutes); + + if clamped != minutes { + self.revoke_edit_access_editor.update(ctx, |editor, ctx| { + editor.set_buffer_text(&clamped.to_string(), ctx); + }); + } + + let new_duration = Duration::from_secs(clamped * 60); + if *SharedSessionSettings::as_ref(ctx).inactivity_period_before_revoking_roles + != new_duration + { + SharedSessionSettings::handle(ctx).update(ctx, |settings, ctx| { + report_if_error!( + settings + .inactivity_period_before_revoking_roles + .set_value(new_duration, ctx) + ); + }); + } + ctx.notify(); + } + + /// Commits a new value for the "warn" duration, clamping it so the + /// revoke -> warn -> end ordering is preserved. + fn commit_warning(&mut self, ctx: &mut ViewContext) { + let text = self.warning_editor.as_ref(ctx).buffer_text(ctx); + let Some(minutes) = Self::parse_minutes(&text) else { + self.is_warning_valid = false; + ctx.notify(); + return; + }; + self.is_warning_valid = true; + + let settings = SharedSessionSettings::as_ref(ctx); + let revoke_minutes = Self::minutes(*settings.inactivity_period_before_revoking_roles); + let end_minutes = Self::minutes(*settings.inactivity_period_before_ending_session); + let clamped = minutes.max(revoke_minutes).min(end_minutes); + + if clamped != minutes { + self.warning_editor.update(ctx, |editor, ctx| { + editor.set_buffer_text(&clamped.to_string(), ctx); + }); + } + + let new_duration = Duration::from_secs(clamped * 60); + if *SharedSessionSettings::as_ref(ctx).inactivity_period_before_warning != new_duration { + SharedSessionSettings::handle(ctx).update(ctx, |settings, ctx| { + report_if_error!( + settings + .inactivity_period_before_warning + .set_value(new_duration, ctx) + ); + }); + } + ctx.notify(); + } + + /// Commits a new value for the "end session" duration, clamping it so the + /// revoke -> warn -> end ordering is preserved. + fn commit_end_session(&mut self, ctx: &mut ViewContext) { + let text = self.end_session_editor.as_ref(ctx).buffer_text(ctx); + let Some(minutes) = Self::parse_minutes(&text) else { + self.is_end_session_valid = false; + ctx.notify(); + return; + }; + self.is_end_session_valid = true; + + let settings = SharedSessionSettings::as_ref(ctx); + let revoke_minutes = Self::minutes(*settings.inactivity_period_before_revoking_roles); + let warning_minutes = Self::minutes(*settings.inactivity_period_before_warning); + let clamped = minutes.max(revoke_minutes).max(warning_minutes); + + if clamped != minutes { + self.end_session_editor.update(ctx, |editor, ctx| { + editor.set_buffer_text(&clamped.to_string(), ctx); + }); + } + + let new_duration = Duration::from_secs(clamped * 60); + if *SharedSessionSettings::as_ref(ctx).inactivity_period_before_ending_session + != new_duration + { + SharedSessionSettings::handle(ctx).update(ctx, |settings, ctx| { + report_if_error!( + settings + .inactivity_period_before_ending_session + .set_value(new_duration, ctx) + ); + }); + } + ctx.notify(); + } + + #[allow(clippy::too_many_arguments)] + fn render_duration_row( + &self, + appearance: &Appearance, + app: &AppContext, + label: &str, + description: &str, + editor: &ViewHandle, + is_valid: bool, + storage_key: &str, + sync_to_cloud: settings::SyncToCloud, + ) -> Box { + let theme = appearance.theme(); + let border_color = if is_valid { + None + } else { + Some(crate::themes::theme::Fill::error().into()) + }; + + let editor_style = UiComponentStyles { + width: Some(48.), + padding: Some(Coords::uniform(5.)), + background: Some(theme.surface_2().into()), + border_color, + ..Default::default() + }; + + let control = Flex::row() + .with_cross_axis_alignment(CrossAxisAlignment::Center) + .with_child( + appearance + .ui_builder() + .text_input(editor.clone()) + .with_style(editor_style) + .build() + .finish(), + ) + .with_child( + Container::new( + Text::new_inline( + "minutes", + appearance.ui_font_family(), + appearance.ui_font_size(), + ) + .with_color(theme.active_ui_text_color().into()) + .finish(), + ) + .with_margin_left(8.) + .finish(), + ) + .finish(); + + render_body_item::( + label.to_string(), + None, + LocalOnlyIconState::for_setting( + storage_key, + sync_to_cloud, + &mut self.local_only_icon_states.borrow_mut(), + app, + ), + ToggleState::Enabled, + appearance, + control, + Some(description.to_string()), + ) + } +} + +impl Entity for SharedSessionInactivityView { + type Event = (); +} + +impl View for SharedSessionInactivityView { + fn ui_name() -> &'static str { + "SharedSessionInactivityView" + } + + fn render(&self, app: &AppContext) -> Box { + let appearance = Appearance::as_ref(app); + + Flex::column() + .with_cross_axis_alignment(CrossAxisAlignment::Stretch) + .with_child(self.render_duration_row( + appearance, + app, + "Revoke edit access after being inactive for", + "Switches everyone you're sharing this session with to read-only after this much inactivity.", + &self.revoke_edit_access_editor, + self.is_revoke_edit_access_valid, + InactivityPeriodBeforeRevokingRoles::storage_key(), + InactivityPeriodBeforeRevokingRoles::sync_to_cloud(), + )) + .with_child(self.render_duration_row( + appearance, + app, + "Warn before ending the session after being inactive for", + "Shows a warning that the shared session is about to end.", + &self.warning_editor, + self.is_warning_valid, + InactivityPeriodBeforeWarning::storage_key(), + InactivityPeriodBeforeWarning::sync_to_cloud(), + )) + .with_child(self.render_duration_row( + appearance, + app, + "End the shared session after being inactive for", + "Automatically ends the shared session and disconnects everyone.", + &self.end_session_editor, + self.is_end_session_valid, + InactivityPeriodBeforeEndingSession::storage_key(), + InactivityPeriodBeforeEndingSession::sync_to_cloud(), + )) + .finish() + } +} + +impl TypedActionView for SharedSessionInactivityView { + type Action = Action; + + fn handle_action(&mut self, action: &Self::Action, ctx: &mut ViewContext) { + match action { + Action::RevokeEditAccessAfterChanged => self.commit_revoke_edit_access(ctx), + Action::WarningAfterChanged => self.commit_warning(ctx), + Action::EndSessionAfterChanged => self.commit_end_session(ctx), + } + } +} diff --git a/app/src/settings_view/features_page.rs b/app/src/settings_view/features_page.rs index b5b545b905e..f23e96aa926 100644 --- a/app/src/settings_view/features_page.rs +++ b/app/src/settings_view/features_page.rs @@ -1403,6 +1403,7 @@ pub struct FeaturesPageView { #[cfg(feature = "local_tty")] startup_shell_view: ViewHandle, undo_close_view: ViewHandle, + shared_session_inactivity_view: ViewHandle, max_block_size_input_editor: ViewHandle, valid_max_block_size: bool, @@ -2491,6 +2492,9 @@ impl FeaturesPageView { let undo_close_view = ctx.add_typed_action_view(features::UndoCloseView::new); + let shared_session_inactivity_view = + ctx.add_typed_action_view(features::SharedSessionInactivityView::new); + let appearance_handle = Appearance::handle(ctx); let width_and_height_editor_options = SingleLineEditorOptions { @@ -2695,6 +2699,7 @@ impl FeaturesPageView { #[cfg(feature = "local_tty")] startup_shell_view, undo_close_view, + shared_session_inactivity_view, max_block_size_input_editor: block_size_editor, valid_max_block_size: true, @@ -2861,6 +2866,7 @@ impl FeaturesPageView { .is_supported_on_current_platform() { session_widgets.push(Box::new(ConfirmCloseSharedSessionWidget::default())); + session_widgets.push(Box::new(SharedSessionInactivityWidget::default())); } let mut keys_widgets: Vec>> = vec![]; @@ -5586,6 +5592,26 @@ impl SettingsWidget for ConfirmCloseSharedSessionWidget { } } +#[derive(Default)] +struct SharedSessionInactivityWidget {} + +impl SettingsWidget for SharedSessionInactivityWidget { + type View = FeaturesPageView; + + fn search_terms(&self) -> &str { + "shared session sharing remote control inactivity idle timeout auto end warning revoke edit access" + } + + fn render( + &self, + view: &Self::View, + _appearance: &Appearance, + _app: &AppContext, + ) -> Box { + ChildView::new(&view.shared_session_inactivity_view).finish() + } +} + #[derive(Default)] struct ExtraMetaKeysWidget { left_switch_state: SwitchStateHandle, diff --git a/app/src/terminal/shared_session/settings.rs b/app/src/terminal/shared_session/settings.rs index ff343d3566e..471c8b5189c 100644 --- a/app/src/terminal/shared_session/settings.rs +++ b/app/src/terminal/shared_session/settings.rs @@ -19,7 +19,9 @@ define_settings_group!(SharedSessionSettings, settings: [ supported_platforms: SupportedPlatforms::ALL, sync_to_cloud: SyncToCloud::Globally(RespectUserSyncSetting::Yes), surface: settings::SettingSurfaces::GUI, - private: true, + private: false, + toml_path: "session_sharing.inactivity.end_session_after_secs", + description: "How long a shared session can be inactive before it is automatically ended, in seconds.", }, inactivity_period_before_warning: InactivityPeriodBeforeWarning { type: Duration, @@ -28,7 +30,9 @@ define_settings_group!(SharedSessionSettings, settings: [ supported_platforms: SupportedPlatforms::ALL, sync_to_cloud: SyncToCloud::Globally(RespectUserSyncSetting::Yes), surface: settings::SettingSurfaces::GUI, - private: true, + private: false, + toml_path: "session_sharing.inactivity.warning_after_secs", + description: "How long a shared session can be inactive before you're warned it's about to end, in seconds.", }, inactivity_period_before_revoking_roles: InactivityPeriodBeforeRevokingRoles { type: Duration, @@ -37,7 +41,9 @@ define_settings_group!(SharedSessionSettings, settings: [ supported_platforms: SupportedPlatforms::ALL, sync_to_cloud: SyncToCloud::Globally(RespectUserSyncSetting::Yes), surface: settings::SettingSurfaces::GUI, - private: true, + private: false, + toml_path: "session_sharing.inactivity.revoke_edit_access_after_secs", + description: "How long a shared session can be inactive before edit access is automatically revoked from everyone you're sharing with, in seconds.", }, // Killswitch: when false, the sharer ignores viewer terminal size reports. viewer_driven_sizing_enabled: ViewerDrivenSizingEnabled { From 10e4c66d82a5471e93fb4f488373df91a5a4b15d Mon Sep 17 00:00:00 2001 From: Oz Date: Tue, 11 Aug 2026 20:59:39 +0000 Subject: [PATCH 2/2] Address review: migration, ordering validation, overflow fix, widget split Adversarial review of #14954 surfaced five findings; all addressed here: 1. Legacy private-store values for the three inactivity settings are now migrated into their new public location via a dedicated one-time migration (its own completion marker, independent of the general SettingsFileMigrationComplete marker which is already set for existing SettingsFile users). Only copies when the public location doesn't already have a value. 2. Added SharedSessionSettings::register_and_enforce_inactivity_ordering, which corrects out-of-order values at every point they become authoritative (initial load/hand-edited file, cloud sync, disk hot-reload), and changed the two derived-interval helpers in settings.rs to use saturating_sub as defense-in-depth against the two latent underflow panics found in the ladder. The ordering comparison is isolated in ladder_phase_order_ok so a future zero-disables-a-phase change only needs to touch that one predicate. 3. parse_shared_session_inactivity_minutes now rejects any value above u64::MAX / 60, preventing the *60-to-seconds conversion from overflowing. 4. Split the single SharedSessionInactivityWidget (one shared search_terms blob covering three rows) into three independent SettingsWidgets (SharedSessionRevokeEditAccessWidget / SharedSessionWarningWidget / SharedSessionEndSessionWidget), each with row-scoped search terms, each backed by its own editor field on FeaturesPageView (mirroring MouseScrollMultiplierWidget) instead of a ChildView-wrapped sub-view, since ChildView's dispatch boundary would have broken action routing for per-row widgets. This also resolves the enum_variant_names lint nonblocking comment and removes the redundant enum-variant doc comments finding, since the standalone Action enum they were attached to no longer exists. 5. N/A - see (4). Added tests: legacy-migration survival/no-clobber/idempotency, ordering correction from storage and cloud sync, a pure saturating_sub regression test, minute-parsing bounds (including the overflow case), clamp correctness/idempotency, and a StubWidget-based filter test proving the three rows are independently searchable. Verified visually with a freshly built and launched instance: all three rows render correctly, 'revoke'/'disconnect' searches now match only their own row, edits persist across Settings modal close/reopen, and out-of-order input is clamped rather than accepted. Co-Authored-By: Warp Agent --- app/src/settings/init.rs | 2 +- app/src/settings_view/features/mod.rs | 3 - .../features/shared_session_inactivity.rs | 454 -------------- app/src/settings_view/features_page.rs | 555 +++++++++++++++++- app/src/settings_view/features_page_tests.rs | 127 ++++ app/src/settings_view/mod_tests.rs | 65 ++ app/src/terminal/shared_session/settings.rs | 187 +++++- .../terminal/shared_session/settings_tests.rs | 285 +++++++++ app/src/test_util/settings.rs | 2 +- 9 files changed, 1205 insertions(+), 475 deletions(-) delete mode 100644 app/src/settings_view/features/shared_session_inactivity.rs create mode 100644 app/src/settings_view/features_page_tests.rs create mode 100644 app/src/terminal/shared_session/settings_tests.rs diff --git a/app/src/settings/init.rs b/app/src/settings/init.rs index de136f0d6bb..0a5c585a331 100644 --- a/app/src/settings/init.rs +++ b/app/src/settings/init.rs @@ -98,7 +98,7 @@ pub fn register_all_settings(ctx: &mut AppContext) { SshSettings::register(ctx); VimBannerSettings::register(ctx); SharedObjectLimitBannerSettings::register(ctx); - SharedSessionSettings::register(ctx); + SharedSessionSettings::register_and_enforce_inactivity_ordering(ctx); WarpDriveSettings::register(ctx); WorkflowAliases::register(ctx); EmacsBindingsSettings::register(ctx); diff --git a/app/src/settings_view/features/mod.rs b/app/src/settings_view/features/mod.rs index 091431d62a4..7c2540741e0 100644 --- a/app/src/settings_view/features/mod.rs +++ b/app/src/settings_view/features/mod.rs @@ -1,6 +1,3 @@ -pub mod shared_session_inactivity; -pub use shared_session_inactivity::SharedSessionInactivityView; - pub mod undo_close; pub use undo_close::UndoCloseView; diff --git a/app/src/settings_view/features/shared_session_inactivity.rs b/app/src/settings_view/features/shared_session_inactivity.rs deleted file mode 100644 index ebc97965b53..00000000000 --- a/app/src/settings_view/features/shared_session_inactivity.rs +++ /dev/null @@ -1,454 +0,0 @@ -use std::cell::RefCell; -use std::collections::HashMap; -use std::time::Duration; - -use settings::Setting; -use warp_errors::report_if_error; -use warpui::elements::{ - Container, CrossAxisAlignment, Flex, MouseStateHandle, ParentElement, Text, -}; -use warpui::ui_components::components::{Coords, UiComponent, UiComponentStyles}; -use warpui::{ - AppContext, Element, Entity, SingletonEntity, TypedActionView, View, ViewContext, ViewHandle, -}; - -use crate::appearance::Appearance; -use crate::editor::{self, EditorView, SingleLineEditorOptions, TextOptions}; -use crate::settings_view::settings_page::{LocalOnlyIconState, ToggleState, render_body_item}; -use crate::terminal::shared_session::settings::{ - InactivityPeriodBeforeEndingSession, InactivityPeriodBeforeRevokingRoles, - InactivityPeriodBeforeWarning, SharedSessionSettings, SharedSessionSettingsChangedEvent, -}; - -/// Minimum allowed value for any of the inactivity durations, in minutes. -const MIN_MINUTES: u64 = 1; - -#[derive(Debug, Clone, Copy)] -#[allow(clippy::enum_variant_names)] -pub enum Action { - /// The "revoke edit access after" duration editor was committed. - RevokeEditAccessAfterChanged, - /// The "warn before ending session after" duration editor was committed. - WarningAfterChanged, - /// The "end session after" duration editor was committed. - EndSessionAfterChanged, -} - -/// A view containing settings that control how long a shared session can sit -/// idle before the sharer's edit access ladder kicks in: edit access is -/// revoked, then a warning is shown, then the session ends. This view only -/// exposes those existing durations for editing; it does not change the -/// underlying inactivity behavior. -pub struct SharedSessionInactivityView { - revoke_edit_access_editor: ViewHandle, - warning_editor: ViewHandle, - end_session_editor: ViewHandle, - is_revoke_edit_access_valid: bool, - is_warning_valid: bool, - is_end_session_valid: bool, - local_only_icon_states: RefCell>, -} - -impl SharedSessionInactivityView { - pub fn new(ctx: &mut ViewContext) -> Self { - let editor_options = SingleLineEditorOptions { - text: TextOptions::ui_font_size(Appearance::as_ref(ctx)), - ..Default::default() - }; - - let revoke_edit_access_editor = - ctx.add_typed_action_view(|ctx| EditorView::single_line(editor_options.clone(), ctx)); - let warning_editor = - ctx.add_typed_action_view(|ctx| EditorView::single_line(editor_options.clone(), ctx)); - let end_session_editor = - ctx.add_typed_action_view(|ctx| EditorView::single_line(editor_options.clone(), ctx)); - - ctx.subscribe_to_model( - &SharedSessionSettings::handle(ctx), - |me, settings, event, ctx| { - match event { - SharedSessionSettingsChangedEvent::InactivityPeriodBeforeRevokingRoles { - .. - } => { - me.revoke_edit_access_editor.update(ctx, |editor, ctx| { - let minutes = Self::minutes( - *settings.as_ref(ctx).inactivity_period_before_revoking_roles, - ); - editor.set_buffer_text(&minutes.to_string(), ctx); - }); - } - SharedSessionSettingsChangedEvent::InactivityPeriodBeforeWarning { .. } => { - me.warning_editor.update(ctx, |editor, ctx| { - let minutes = Self::minutes( - *settings.as_ref(ctx).inactivity_period_before_warning, - ); - editor.set_buffer_text(&minutes.to_string(), ctx); - }); - } - SharedSessionSettingsChangedEvent::InactivityPeriodBeforeEndingSession { - .. - } => { - me.end_session_editor.update(ctx, |editor, ctx| { - let minutes = Self::minutes( - *settings.as_ref(ctx).inactivity_period_before_ending_session, - ); - editor.set_buffer_text(&minutes.to_string(), ctx); - }); - } - _ => {} - } - ctx.notify(); - }, - ); - - ctx.subscribe_to_view(&revoke_edit_access_editor, move |me, _, event, ctx| { - me.handle_revoke_edit_access_editor_event(event, ctx); - }); - ctx.subscribe_to_view(&warning_editor, move |me, _, event, ctx| { - me.handle_warning_editor_event(event, ctx); - }); - ctx.subscribe_to_view(&end_session_editor, move |me, _, event, ctx| { - me.handle_end_session_editor_event(event, ctx); - }); - - let settings = SharedSessionSettings::as_ref(ctx); - let revoke_minutes = Self::minutes(*settings.inactivity_period_before_revoking_roles); - let warning_minutes = Self::minutes(*settings.inactivity_period_before_warning); - let end_minutes = Self::minutes(*settings.inactivity_period_before_ending_session); - - revoke_edit_access_editor.update(ctx, |editor, ctx| { - editor.set_buffer_text(&revoke_minutes.to_string(), ctx); - }); - warning_editor.update(ctx, |editor, ctx| { - editor.set_buffer_text(&warning_minutes.to_string(), ctx); - }); - end_session_editor.update(ctx, |editor, ctx| { - editor.set_buffer_text(&end_minutes.to_string(), ctx); - }); - - Self { - revoke_edit_access_editor, - warning_editor, - end_session_editor, - is_revoke_edit_access_valid: true, - is_warning_valid: true, - is_end_session_valid: true, - local_only_icon_states: Default::default(), - } - } - - /// Parses user-entered text into a positive number of minutes, returning - /// `None` if the text isn't a valid, positive integer. - fn parse_minutes(text: &str) -> Option { - text.trim() - .parse::() - .ok() - .filter(|&minutes| minutes >= MIN_MINUTES) - } - - /// Converts a duration to whole minutes for display, rounding up so that - /// a duration is never displayed as zero minutes. - fn minutes(duration: Duration) -> u64 { - duration.as_secs().div_ceil(60).max(MIN_MINUTES) - } - - fn handle_revoke_edit_access_editor_event( - &mut self, - event: &editor::Event, - ctx: &mut ViewContext, - ) { - use editor::Event; - match event { - Event::Edited(_) => { - let text = self.revoke_edit_access_editor.as_ref(ctx).buffer_text(ctx); - let is_valid = Self::parse_minutes(&text).is_some(); - if is_valid != self.is_revoke_edit_access_valid { - self.is_revoke_edit_access_valid = is_valid; - ctx.notify(); - } - } - Event::Blurred | Event::Enter => { - self.handle_action(&Action::RevokeEditAccessAfterChanged, ctx); - } - _ => (), - } - } - - fn handle_warning_editor_event(&mut self, event: &editor::Event, ctx: &mut ViewContext) { - use editor::Event; - match event { - Event::Edited(_) => { - let text = self.warning_editor.as_ref(ctx).buffer_text(ctx); - let is_valid = Self::parse_minutes(&text).is_some(); - if is_valid != self.is_warning_valid { - self.is_warning_valid = is_valid; - ctx.notify(); - } - } - Event::Blurred | Event::Enter => { - self.handle_action(&Action::WarningAfterChanged, ctx); - } - _ => (), - } - } - - fn handle_end_session_editor_event( - &mut self, - event: &editor::Event, - ctx: &mut ViewContext, - ) { - use editor::Event; - match event { - Event::Edited(_) => { - let text = self.end_session_editor.as_ref(ctx).buffer_text(ctx); - let is_valid = Self::parse_minutes(&text).is_some(); - if is_valid != self.is_end_session_valid { - self.is_end_session_valid = is_valid; - ctx.notify(); - } - } - Event::Blurred | Event::Enter => { - self.handle_action(&Action::EndSessionAfterChanged, ctx); - } - _ => (), - } - } - - /// Commits a new value for the "revoke edit access" duration, clamping it - /// so the revoke -> warn -> end ordering is preserved. The other two - /// durations are never modified as a result of this edit. - fn commit_revoke_edit_access(&mut self, ctx: &mut ViewContext) { - let text = self.revoke_edit_access_editor.as_ref(ctx).buffer_text(ctx); - let Some(minutes) = Self::parse_minutes(&text) else { - self.is_revoke_edit_access_valid = false; - ctx.notify(); - return; - }; - self.is_revoke_edit_access_valid = true; - - let settings = SharedSessionSettings::as_ref(ctx); - let warning_minutes = Self::minutes(*settings.inactivity_period_before_warning); - let end_minutes = Self::minutes(*settings.inactivity_period_before_ending_session); - let clamped = minutes.min(warning_minutes).min(end_minutes); - - if clamped != minutes { - self.revoke_edit_access_editor.update(ctx, |editor, ctx| { - editor.set_buffer_text(&clamped.to_string(), ctx); - }); - } - - let new_duration = Duration::from_secs(clamped * 60); - if *SharedSessionSettings::as_ref(ctx).inactivity_period_before_revoking_roles - != new_duration - { - SharedSessionSettings::handle(ctx).update(ctx, |settings, ctx| { - report_if_error!( - settings - .inactivity_period_before_revoking_roles - .set_value(new_duration, ctx) - ); - }); - } - ctx.notify(); - } - - /// Commits a new value for the "warn" duration, clamping it so the - /// revoke -> warn -> end ordering is preserved. - fn commit_warning(&mut self, ctx: &mut ViewContext) { - let text = self.warning_editor.as_ref(ctx).buffer_text(ctx); - let Some(minutes) = Self::parse_minutes(&text) else { - self.is_warning_valid = false; - ctx.notify(); - return; - }; - self.is_warning_valid = true; - - let settings = SharedSessionSettings::as_ref(ctx); - let revoke_minutes = Self::minutes(*settings.inactivity_period_before_revoking_roles); - let end_minutes = Self::minutes(*settings.inactivity_period_before_ending_session); - let clamped = minutes.max(revoke_minutes).min(end_minutes); - - if clamped != minutes { - self.warning_editor.update(ctx, |editor, ctx| { - editor.set_buffer_text(&clamped.to_string(), ctx); - }); - } - - let new_duration = Duration::from_secs(clamped * 60); - if *SharedSessionSettings::as_ref(ctx).inactivity_period_before_warning != new_duration { - SharedSessionSettings::handle(ctx).update(ctx, |settings, ctx| { - report_if_error!( - settings - .inactivity_period_before_warning - .set_value(new_duration, ctx) - ); - }); - } - ctx.notify(); - } - - /// Commits a new value for the "end session" duration, clamping it so the - /// revoke -> warn -> end ordering is preserved. - fn commit_end_session(&mut self, ctx: &mut ViewContext) { - let text = self.end_session_editor.as_ref(ctx).buffer_text(ctx); - let Some(minutes) = Self::parse_minutes(&text) else { - self.is_end_session_valid = false; - ctx.notify(); - return; - }; - self.is_end_session_valid = true; - - let settings = SharedSessionSettings::as_ref(ctx); - let revoke_minutes = Self::minutes(*settings.inactivity_period_before_revoking_roles); - let warning_minutes = Self::minutes(*settings.inactivity_period_before_warning); - let clamped = minutes.max(revoke_minutes).max(warning_minutes); - - if clamped != minutes { - self.end_session_editor.update(ctx, |editor, ctx| { - editor.set_buffer_text(&clamped.to_string(), ctx); - }); - } - - let new_duration = Duration::from_secs(clamped * 60); - if *SharedSessionSettings::as_ref(ctx).inactivity_period_before_ending_session - != new_duration - { - SharedSessionSettings::handle(ctx).update(ctx, |settings, ctx| { - report_if_error!( - settings - .inactivity_period_before_ending_session - .set_value(new_duration, ctx) - ); - }); - } - ctx.notify(); - } - - #[allow(clippy::too_many_arguments)] - fn render_duration_row( - &self, - appearance: &Appearance, - app: &AppContext, - label: &str, - description: &str, - editor: &ViewHandle, - is_valid: bool, - storage_key: &str, - sync_to_cloud: settings::SyncToCloud, - ) -> Box { - let theme = appearance.theme(); - let border_color = if is_valid { - None - } else { - Some(crate::themes::theme::Fill::error().into()) - }; - - let editor_style = UiComponentStyles { - width: Some(48.), - padding: Some(Coords::uniform(5.)), - background: Some(theme.surface_2().into()), - border_color, - ..Default::default() - }; - - let control = Flex::row() - .with_cross_axis_alignment(CrossAxisAlignment::Center) - .with_child( - appearance - .ui_builder() - .text_input(editor.clone()) - .with_style(editor_style) - .build() - .finish(), - ) - .with_child( - Container::new( - Text::new_inline( - "minutes", - appearance.ui_font_family(), - appearance.ui_font_size(), - ) - .with_color(theme.active_ui_text_color().into()) - .finish(), - ) - .with_margin_left(8.) - .finish(), - ) - .finish(); - - render_body_item::( - label.to_string(), - None, - LocalOnlyIconState::for_setting( - storage_key, - sync_to_cloud, - &mut self.local_only_icon_states.borrow_mut(), - app, - ), - ToggleState::Enabled, - appearance, - control, - Some(description.to_string()), - ) - } -} - -impl Entity for SharedSessionInactivityView { - type Event = (); -} - -impl View for SharedSessionInactivityView { - fn ui_name() -> &'static str { - "SharedSessionInactivityView" - } - - fn render(&self, app: &AppContext) -> Box { - let appearance = Appearance::as_ref(app); - - Flex::column() - .with_cross_axis_alignment(CrossAxisAlignment::Stretch) - .with_child(self.render_duration_row( - appearance, - app, - "Revoke edit access after being inactive for", - "Switches everyone you're sharing this session with to read-only after this much inactivity.", - &self.revoke_edit_access_editor, - self.is_revoke_edit_access_valid, - InactivityPeriodBeforeRevokingRoles::storage_key(), - InactivityPeriodBeforeRevokingRoles::sync_to_cloud(), - )) - .with_child(self.render_duration_row( - appearance, - app, - "Warn before ending the session after being inactive for", - "Shows a warning that the shared session is about to end.", - &self.warning_editor, - self.is_warning_valid, - InactivityPeriodBeforeWarning::storage_key(), - InactivityPeriodBeforeWarning::sync_to_cloud(), - )) - .with_child(self.render_duration_row( - appearance, - app, - "End the shared session after being inactive for", - "Automatically ends the shared session and disconnects everyone.", - &self.end_session_editor, - self.is_end_session_valid, - InactivityPeriodBeforeEndingSession::storage_key(), - InactivityPeriodBeforeEndingSession::sync_to_cloud(), - )) - .finish() - } -} - -impl TypedActionView for SharedSessionInactivityView { - type Action = Action; - - fn handle_action(&mut self, action: &Self::Action, ctx: &mut ViewContext) { - match action { - Action::RevokeEditAccessAfterChanged => self.commit_revoke_edit_access(ctx), - Action::WarningAfterChanged => self.commit_warning(ctx), - Action::EndSessionAfterChanged => self.commit_end_session(ctx), - } - } -} diff --git a/app/src/settings_view/features_page.rs b/app/src/settings_view/features_page.rs index f23e96aa926..8beeb79bc3d 100644 --- a/app/src/settings_view/features_page.rs +++ b/app/src/settings_view/features_page.rs @@ -100,6 +100,10 @@ use crate::terminal::settings::{ AsyncFindEnabled, MaximumGridSize, Osc52ClipboardAccess, Osc52ClipboardAccessSetting, ShowTerminalZeroStateBlock, TerminalSettings, TerminalSettingsChangedEvent, UseAudibleBell, }; +use crate::terminal::shared_session::settings::{ + InactivityPeriodBeforeEndingSession, InactivityPeriodBeforeRevokingRoles, + InactivityPeriodBeforeWarning, SharedSessionSettings, SharedSessionSettingsChangedEvent, +}; use crate::terminal::{BlockListSettings, PreserveInputFocusOnBlockSelection, SnackbarEnabled}; use crate::undo_close::UndoCloseSettings; use crate::user_config::{WarpConfig, WarpConfigUpdateEvent}; @@ -111,6 +115,10 @@ use crate::workspace::WorkspaceAction; use crate::workspace::tab_settings::{NewTabPlacement, TabSettings, TabSettingsChangedEvent}; use crate::{GlobalResourceHandles, send_telemetry_from_ctx, themes}; +#[cfg(test)] +#[path = "features_page_tests.rs"] +mod features_page_tests; + cfg_if::cfg_if! { if #[cfg(target_os = "macos")] { static EXTRA_META_KEYS_LEFT_TEXT: &str = "Left Option key is Meta"; @@ -811,6 +819,9 @@ pub enum FeaturesPageAction { ToggleAgentInAppNotifications, MakeWarpDefaultTerminal, SetCodeEditorLineNumberMode(CodeEditorLineNumberMode), + SetSharedSessionRevokeEditAccessAfter, + SetSharedSessionWarningAfter, + SetSharedSessionEndSessionAfter, } lazy_static! { @@ -845,8 +856,68 @@ const MOUSE_SCROLL_EDITOR_WIDTH: f32 = 40.; const MIN_MOUSE_SCROLL_MULTIPLIER: f32 = 1.0; const MAX_MOUSE_SCROLL_MULTIPLIER: f32 = 20.0; +const SHARED_SESSION_INACTIVITY_MIN_MINUTES: u64 = 1; +/// Largest value that can be multiplied by 60 (to convert to seconds) without +/// overflowing `u64`. Rejecting anything above this bound at parse time keeps +/// `Duration::from_secs(minutes * 60)` from wrapping (release) or panicking +/// (debug). +const SHARED_SESSION_INACTIVITY_MAX_MINUTES: u64 = u64::MAX / 60; + +const SHARED_SESSION_INACTIVITY_EDITOR_WIDTH: f32 = 48.; + const TAB_KEYSTROKE_STR: &str = "Tab"; +/// Parses user-entered text into a positive number of minutes, bounded so +/// converting back to seconds can never overflow. Returns `None` for +/// anything else (empty, non-numeric, zero, or too large). +fn parse_shared_session_inactivity_minutes(text: &str) -> Option { + text.trim().parse::().ok().filter(|&minutes| { + (SHARED_SESSION_INACTIVITY_MIN_MINUTES..=SHARED_SESSION_INACTIVITY_MAX_MINUTES) + .contains(&minutes) + }) +} + +/// Converts a duration to whole minutes for display, rounding up so a +/// duration is never displayed as zero minutes. +fn shared_session_inactivity_minutes(duration: Duration) -> u64 { + duration + .as_secs() + .div_ceil(60) + .max(SHARED_SESSION_INACTIVITY_MIN_MINUTES) +} + +/// Clamps a newly-committed "revoke edit access after" value so it never +/// exceeds the other two (currently) configured durations, preserving +/// `revoke <= warn <= end`. Leaves the other two settings untouched. +fn clamp_shared_session_revoke_minutes( + minutes: u64, + warning_minutes: u64, + end_minutes: u64, +) -> u64 { + minutes.min(warning_minutes).min(end_minutes) +} + +/// Clamps a newly-committed "warn before ending" value between the other two +/// (currently) configured durations, preserving `revoke <= warn <= end`. +fn clamp_shared_session_warning_minutes( + minutes: u64, + revoke_minutes: u64, + end_minutes: u64, +) -> u64 { + minutes.max(revoke_minutes).min(end_minutes) +} + +/// Clamps a newly-committed "end session after" value so it's never less +/// than the other two (currently) configured durations, preserving +/// `revoke <= warn <= end`. +fn clamp_shared_session_end_minutes( + minutes: u64, + revoke_minutes: u64, + warning_minutes: u64, +) -> u64 { + minutes.max(revoke_minutes).max(warning_minutes) +} + /// Function to get maximum value for max grid size: 10 million for dogfood/dev builds, /// 1 million for release builds. /// @@ -1342,6 +1413,33 @@ impl FeaturesPageAction { action: "ToggleAsyncFind".to_string(), value: to_string(*TerminalSettings::as_ref(ctx).async_find_enabled), }, + Self::SetSharedSessionRevokeEditAccessAfter => TelemetryEvent::FeaturesPageAction { + action: "SetSharedSessionRevokeEditAccessAfter".to_string(), + value: format!( + "{}s", + SharedSessionSettings::as_ref(ctx) + .inactivity_period_before_revoking_roles + .as_secs() + ), + }, + Self::SetSharedSessionWarningAfter => TelemetryEvent::FeaturesPageAction { + action: "SetSharedSessionWarningAfter".to_string(), + value: format!( + "{}s", + SharedSessionSettings::as_ref(ctx) + .inactivity_period_before_warning + .as_secs() + ), + }, + Self::SetSharedSessionEndSessionAfter => TelemetryEvent::FeaturesPageAction { + action: "SetSharedSessionEndSessionAfter".to_string(), + value: format!( + "{}s", + SharedSessionSettings::as_ref(ctx) + .inactivity_period_before_ending_session + .as_secs() + ), + }, } } } @@ -1403,7 +1501,6 @@ pub struct FeaturesPageView { #[cfg(feature = "local_tty")] startup_shell_view: ViewHandle, undo_close_view: ViewHandle, - shared_session_inactivity_view: ViewHandle, max_block_size_input_editor: ViewHandle, valid_max_block_size: bool, @@ -1411,6 +1508,13 @@ pub struct FeaturesPageView { mouse_scroll_input_editor: ViewHandle, valid_mouse_scroll_multiplier: bool, + shared_session_revoke_edit_access_editor: ViewHandle, + valid_shared_session_revoke_edit_access: bool, + shared_session_warning_editor: ViewHandle, + valid_shared_session_warning: bool, + shared_session_end_session_editor: ViewHandle, + valid_shared_session_end_session: bool, + #[cfg(feature = "local_fs")] external_editor_view: ViewHandle, word_boundary_editor: ViewHandle, @@ -2191,6 +2295,11 @@ impl TypedActionView for FeaturesPageView { ); }); } + SetSharedSessionRevokeEditAccessAfter => { + self.commit_shared_session_revoke_edit_access(ctx) + } + SetSharedSessionWarningAfter => self.commit_shared_session_warning(ctx), + SetSharedSessionEndSessionAfter => self.commit_shared_session_end_session(ctx), } send_telemetry_from_ctx!(action.telemetry_event(ctx), ctx); @@ -2273,6 +2382,45 @@ impl FeaturesPageView { ctx.subscribe_to_model(&KeysSettings::handle(ctx), |me, _, _, ctx| { me.handle_hotkey_settings_update(ctx); }); + ctx.subscribe_to_model( + &SharedSessionSettings::handle(ctx), + |me, settings, event, ctx| { + match event { + SharedSessionSettingsChangedEvent::InactivityPeriodBeforeRevokingRoles { + .. + } => { + me.shared_session_revoke_edit_access_editor + .update(ctx, |editor, ctx| { + let minutes = shared_session_inactivity_minutes( + *settings.as_ref(ctx).inactivity_period_before_revoking_roles, + ); + editor.set_buffer_text(&minutes.to_string(), ctx); + }); + } + SharedSessionSettingsChangedEvent::InactivityPeriodBeforeWarning { .. } => { + me.shared_session_warning_editor.update(ctx, |editor, ctx| { + let minutes = shared_session_inactivity_minutes( + *settings.as_ref(ctx).inactivity_period_before_warning, + ); + editor.set_buffer_text(&minutes.to_string(), ctx); + }); + } + SharedSessionSettingsChangedEvent::InactivityPeriodBeforeEndingSession { + .. + } => { + me.shared_session_end_session_editor + .update(ctx, |editor, ctx| { + let minutes = shared_session_inactivity_minutes( + *settings.as_ref(ctx).inactivity_period_before_ending_session, + ); + editor.set_buffer_text(&minutes.to_string(), ctx); + }); + } + _ => {} + } + ctx.notify(); + }, + ); ctx.subscribe_to_model(&SessionSettings::handle(ctx), |me, _, event, ctx| { match event { SessionSettingsChangedEvent::Notifications { .. } => { @@ -2492,9 +2640,6 @@ impl FeaturesPageView { let undo_close_view = ctx.add_typed_action_view(features::UndoCloseView::new); - let shared_session_inactivity_view = - ctx.add_typed_action_view(features::SharedSessionInactivityView::new); - let appearance_handle = Appearance::handle(ctx); let width_and_height_editor_options = SingleLineEditorOptions { @@ -2665,6 +2810,50 @@ impl FeaturesPageView { } }); + let shared_session_settings = SharedSessionSettings::as_ref(ctx); + let shared_session_revoke_edit_access_minutes = shared_session_inactivity_minutes( + *shared_session_settings.inactivity_period_before_revoking_roles, + ); + let shared_session_warning_minutes = shared_session_inactivity_minutes( + *shared_session_settings.inactivity_period_before_warning, + ); + let shared_session_end_session_minutes = shared_session_inactivity_minutes( + *shared_session_settings.inactivity_period_before_ending_session, + ); + + let shared_session_revoke_edit_access_editor = ctx.add_typed_action_view(|ctx| { + EditorView::single_line(width_and_height_editor_options.clone(), ctx) + }); + shared_session_revoke_edit_access_editor.update(ctx, |editor, ctx| { + editor.set_buffer_text(&shared_session_revoke_edit_access_minutes.to_string(), ctx); + }); + ctx.subscribe_to_view( + &shared_session_revoke_edit_access_editor, + |me, _, event, ctx| { + me.handle_shared_session_revoke_edit_access_editor_event(event, ctx); + }, + ); + + let shared_session_warning_editor = ctx.add_typed_action_view(|ctx| { + EditorView::single_line(width_and_height_editor_options.clone(), ctx) + }); + shared_session_warning_editor.update(ctx, |editor, ctx| { + editor.set_buffer_text(&shared_session_warning_minutes.to_string(), ctx); + }); + ctx.subscribe_to_view(&shared_session_warning_editor, |me, _, event, ctx| { + me.handle_shared_session_warning_editor_event(event, ctx); + }); + + let shared_session_end_session_editor = ctx.add_typed_action_view(|ctx| { + EditorView::single_line(width_and_height_editor_options.clone(), ctx) + }); + shared_session_end_session_editor.update(ctx, |editor, ctx| { + editor.set_buffer_text(&shared_session_end_session_minutes.to_string(), ctx); + }); + ctx.subscribe_to_view(&shared_session_end_session_editor, |me, _, event, ctx| { + me.handle_shared_session_end_session_editor_event(event, ctx); + }); + let mut features_page_view = FeaturesPageView { page: Self::build_page(ctx), global_resource_handles, @@ -2699,7 +2888,6 @@ impl FeaturesPageView { #[cfg(feature = "local_tty")] startup_shell_view, undo_close_view, - shared_session_inactivity_view, max_block_size_input_editor: block_size_editor, valid_max_block_size: true, @@ -2723,6 +2911,13 @@ impl FeaturesPageView { mouse_scroll_input_editor, valid_mouse_scroll_multiplier: true, + shared_session_revoke_edit_access_editor, + valid_shared_session_revoke_edit_access: true, + shared_session_warning_editor, + valid_shared_session_warning: true, + shared_session_end_session_editor, + valid_shared_session_end_session: true, + #[cfg(any(target_os = "linux", target_os = "freebsd"))] force_x11_changed: false, gpu_power_preference_changed: false, @@ -2866,7 +3061,9 @@ impl FeaturesPageView { .is_supported_on_current_platform() { session_widgets.push(Box::new(ConfirmCloseSharedSessionWidget::default())); - session_widgets.push(Box::new(SharedSessionInactivityWidget::default())); + session_widgets.push(Box::new(SharedSessionRevokeEditAccessWidget::default())); + session_widgets.push(Box::new(SharedSessionWarningWidget::default())); + session_widgets.push(Box::new(SharedSessionEndSessionWidget::default())); } let mut keys_widgets: Vec>> = vec![]; @@ -3396,6 +3593,197 @@ impl FeaturesPageView { } } + fn handle_shared_session_revoke_edit_access_editor_event( + &mut self, + event: &EditorEvent, + ctx: &mut ViewContext, + ) { + match event { + EditorEvent::Edited(_) => { + let text = self + .shared_session_revoke_edit_access_editor + .as_ref(ctx) + .buffer_text(ctx); + let is_valid = parse_shared_session_inactivity_minutes(&text).is_some(); + if is_valid != self.valid_shared_session_revoke_edit_access { + self.valid_shared_session_revoke_edit_access = is_valid; + ctx.notify(); + } + } + EditorEvent::Enter | EditorEvent::Blurred => { + self.commit_shared_session_revoke_edit_access(ctx) + } + EditorEvent::Escape => ctx.emit(FeaturesSettingsPageEvent::FocusModal), + _ => {} + } + } + + fn commit_shared_session_revoke_edit_access(&mut self, ctx: &mut ViewContext) { + let text = self + .shared_session_revoke_edit_access_editor + .as_ref(ctx) + .buffer_text(ctx); + let Some(minutes) = parse_shared_session_inactivity_minutes(&text) else { + self.valid_shared_session_revoke_edit_access = false; + ctx.notify(); + return; + }; + self.valid_shared_session_revoke_edit_access = true; + + let settings = SharedSessionSettings::as_ref(ctx); + let warning_minutes = + shared_session_inactivity_minutes(*settings.inactivity_period_before_warning); + let end_minutes = + shared_session_inactivity_minutes(*settings.inactivity_period_before_ending_session); + let clamped = clamp_shared_session_revoke_minutes(minutes, warning_minutes, end_minutes); + + if clamped != minutes { + self.shared_session_revoke_edit_access_editor + .update(ctx, |editor, ctx| { + editor.set_buffer_text(&clamped.to_string(), ctx); + }); + } + + let new_duration = Duration::from_secs(clamped * 60); + if *SharedSessionSettings::as_ref(ctx).inactivity_period_before_revoking_roles + != new_duration + { + SharedSessionSettings::handle(ctx).update(ctx, |settings, ctx| { + report_if_error!( + settings + .inactivity_period_before_revoking_roles + .set_value(new_duration, ctx) + ); + }); + } + } + + fn handle_shared_session_warning_editor_event( + &mut self, + event: &EditorEvent, + ctx: &mut ViewContext, + ) { + match event { + EditorEvent::Edited(_) => { + let text = self + .shared_session_warning_editor + .as_ref(ctx) + .buffer_text(ctx); + let is_valid = parse_shared_session_inactivity_minutes(&text).is_some(); + if is_valid != self.valid_shared_session_warning { + self.valid_shared_session_warning = is_valid; + ctx.notify(); + } + } + EditorEvent::Enter | EditorEvent::Blurred => self.commit_shared_session_warning(ctx), + EditorEvent::Escape => ctx.emit(FeaturesSettingsPageEvent::FocusModal), + _ => {} + } + } + + fn commit_shared_session_warning(&mut self, ctx: &mut ViewContext) { + let text = self + .shared_session_warning_editor + .as_ref(ctx) + .buffer_text(ctx); + let Some(minutes) = parse_shared_session_inactivity_minutes(&text) else { + self.valid_shared_session_warning = false; + ctx.notify(); + return; + }; + self.valid_shared_session_warning = true; + + let settings = SharedSessionSettings::as_ref(ctx); + let revoke_minutes = + shared_session_inactivity_minutes(*settings.inactivity_period_before_revoking_roles); + let end_minutes = + shared_session_inactivity_minutes(*settings.inactivity_period_before_ending_session); + let clamped = clamp_shared_session_warning_minutes(minutes, revoke_minutes, end_minutes); + + if clamped != minutes { + self.shared_session_warning_editor + .update(ctx, |editor, ctx| { + editor.set_buffer_text(&clamped.to_string(), ctx); + }); + } + + let new_duration = Duration::from_secs(clamped * 60); + if *SharedSessionSettings::as_ref(ctx).inactivity_period_before_warning != new_duration { + SharedSessionSettings::handle(ctx).update(ctx, |settings, ctx| { + report_if_error!( + settings + .inactivity_period_before_warning + .set_value(new_duration, ctx) + ); + }); + } + } + + fn handle_shared_session_end_session_editor_event( + &mut self, + event: &EditorEvent, + ctx: &mut ViewContext, + ) { + match event { + EditorEvent::Edited(_) => { + let text = self + .shared_session_end_session_editor + .as_ref(ctx) + .buffer_text(ctx); + let is_valid = parse_shared_session_inactivity_minutes(&text).is_some(); + if is_valid != self.valid_shared_session_end_session { + self.valid_shared_session_end_session = is_valid; + ctx.notify(); + } + } + EditorEvent::Enter | EditorEvent::Blurred => { + self.commit_shared_session_end_session(ctx) + } + EditorEvent::Escape => ctx.emit(FeaturesSettingsPageEvent::FocusModal), + _ => {} + } + } + + fn commit_shared_session_end_session(&mut self, ctx: &mut ViewContext) { + let text = self + .shared_session_end_session_editor + .as_ref(ctx) + .buffer_text(ctx); + let Some(minutes) = parse_shared_session_inactivity_minutes(&text) else { + self.valid_shared_session_end_session = false; + ctx.notify(); + return; + }; + self.valid_shared_session_end_session = true; + + let settings = SharedSessionSettings::as_ref(ctx); + let revoke_minutes = + shared_session_inactivity_minutes(*settings.inactivity_period_before_revoking_roles); + let warning_minutes = + shared_session_inactivity_minutes(*settings.inactivity_period_before_warning); + let clamped = clamp_shared_session_end_minutes(minutes, revoke_minutes, warning_minutes); + + if clamped != minutes { + self.shared_session_end_session_editor + .update(ctx, |editor, ctx| { + editor.set_buffer_text(&clamped.to_string(), ctx); + }); + } + + let new_duration = Duration::from_secs(clamped * 60); + if *SharedSessionSettings::as_ref(ctx).inactivity_period_before_ending_session + != new_duration + { + SharedSessionSettings::handle(ctx).update(ctx, |settings, ctx| { + report_if_error!( + settings + .inactivity_period_before_ending_session + .set_value(new_duration, ctx) + ); + }); + } + } + fn set_height_ratio(&mut self, ctx: &mut ViewContext) { let user_input = self.quake_mode_height_editor.as_ref(ctx).buffer_text(ctx); @@ -5592,23 +5980,166 @@ impl SettingsWidget for ConfirmCloseSharedSessionWidget { } } +/// Renders one shared-session inactivity duration row: a minute input, the +/// word "minutes", and a description of what the duration controls. +#[allow(clippy::too_many_arguments)] +fn render_shared_session_inactivity_row( + view: &FeaturesPageView, + appearance: &Appearance, + app: &AppContext, + label: &str, + description: &str, + editor: &ViewHandle, + is_valid: bool, + storage_key: &str, + sync_to_cloud: settings::SyncToCloud, +) -> Box { + let theme = appearance.theme(); + let border_color = if is_valid { + None + } else { + Some(themes::theme::Fill::error().into()) + }; + + let editor_style = UiComponentStyles { + width: Some(SHARED_SESSION_INACTIVITY_EDITOR_WIDTH), + padding: Some(Coords::uniform(5.)), + background: Some(theme.surface_2().into()), + border_color, + ..Default::default() + }; + + let control = Flex::row() + .with_cross_axis_alignment(CrossAxisAlignment::Center) + .with_child( + appearance + .ui_builder() + .text_input(editor.clone()) + .with_style(editor_style) + .build() + .finish(), + ) + .with_child( + Container::new( + Text::new_inline( + "minutes", + appearance.ui_font_family(), + appearance.ui_font_size(), + ) + .with_color(theme.active_ui_text_color().into()) + .finish(), + ) + .with_margin_left(8.) + .finish(), + ) + .finish(); + + render_body_item::( + label.to_string(), + None, + LocalOnlyIconState::for_setting( + storage_key, + sync_to_cloud, + &mut view + .button_mouse_states + .local_only_icon_tooltip_states + .borrow_mut(), + app, + ), + ToggleState::Enabled, + appearance, + control, + Some(description.to_string()), + ) +} + +#[derive(Default)] +struct SharedSessionRevokeEditAccessWidget {} + +impl SettingsWidget for SharedSessionRevokeEditAccessWidget { + type View = FeaturesPageView; + + fn search_terms(&self) -> &str { + "shared session sharing remote control inactivity idle timeout revoke edit access read-only" + } + + fn render( + &self, + view: &Self::View, + appearance: &Appearance, + app: &AppContext, + ) -> Box { + render_shared_session_inactivity_row( + view, + appearance, + app, + "Revoke edit access after being inactive for", + "Switches everyone you're sharing this session with to read-only after this much inactivity.", + &view.shared_session_revoke_edit_access_editor, + view.valid_shared_session_revoke_edit_access, + InactivityPeriodBeforeRevokingRoles::storage_key(), + InactivityPeriodBeforeRevokingRoles::sync_to_cloud(), + ) + } +} + #[derive(Default)] -struct SharedSessionInactivityWidget {} +struct SharedSessionWarningWidget {} -impl SettingsWidget for SharedSessionInactivityWidget { +impl SettingsWidget for SharedSessionWarningWidget { type View = FeaturesPageView; fn search_terms(&self) -> &str { - "shared session sharing remote control inactivity idle timeout auto end warning revoke edit access" + "shared session sharing remote control inactivity idle timeout warn warning ending" } fn render( &self, view: &Self::View, - _appearance: &Appearance, - _app: &AppContext, + appearance: &Appearance, + app: &AppContext, ) -> Box { - ChildView::new(&view.shared_session_inactivity_view).finish() + render_shared_session_inactivity_row( + view, + appearance, + app, + "Warn before ending the session after being inactive for", + "Shows a warning that the shared session is about to end.", + &view.shared_session_warning_editor, + view.valid_shared_session_warning, + InactivityPeriodBeforeWarning::storage_key(), + InactivityPeriodBeforeWarning::sync_to_cloud(), + ) + } +} + +#[derive(Default)] +struct SharedSessionEndSessionWidget {} + +impl SettingsWidget for SharedSessionEndSessionWidget { + type View = FeaturesPageView; + + fn search_terms(&self) -> &str { + "shared session sharing remote control inactivity idle timeout end disconnect" + } + + fn render( + &self, + view: &Self::View, + appearance: &Appearance, + app: &AppContext, + ) -> Box { + render_shared_session_inactivity_row( + view, + appearance, + app, + "End the shared session after being inactive for", + "Automatically ends the shared session and disconnects everyone.", + &view.shared_session_end_session_editor, + view.valid_shared_session_end_session, + InactivityPeriodBeforeEndingSession::storage_key(), + InactivityPeriodBeforeEndingSession::sync_to_cloud(), + ) } } diff --git a/app/src/settings_view/features_page_tests.rs b/app/src/settings_view/features_page_tests.rs new file mode 100644 index 00000000000..d1c5bb9626d --- /dev/null +++ b/app/src/settings_view/features_page_tests.rs @@ -0,0 +1,127 @@ +use std::time::Duration; + +use super::{ + SHARED_SESSION_INACTIVITY_MAX_MINUTES, clamp_shared_session_end_minutes, + clamp_shared_session_revoke_minutes, clamp_shared_session_warning_minutes, + parse_shared_session_inactivity_minutes, shared_session_inactivity_minutes, +}; + +#[test] +fn parse_rejects_zero() { + assert_eq!(parse_shared_session_inactivity_minutes("0"), None); +} + +#[test] +fn parse_rejects_non_numeric() { + assert_eq!(parse_shared_session_inactivity_minutes("abc"), None); + assert_eq!(parse_shared_session_inactivity_minutes(""), None); + assert_eq!(parse_shared_session_inactivity_minutes("-5"), None); + assert_eq!(parse_shared_session_inactivity_minutes("3.5"), None); +} + +#[test] +fn parse_accepts_positive_values_within_bounds() { + assert_eq!(parse_shared_session_inactivity_minutes("1"), Some(1)); + assert_eq!(parse_shared_session_inactivity_minutes("30"), Some(30)); + assert_eq!(parse_shared_session_inactivity_minutes(" 42 "), Some(42)); +} + +/// Regression test for review finding 3: `parse_shared_session_inactivity_minutes` must +/// reject values large enough that `minutes * 60` would overflow `u64` +/// (`307445734561825861` minutes wraps to 44 seconds in release and panics in debug). +#[test] +fn parse_rejects_values_that_would_overflow_when_converted_to_seconds() { + assert_eq!( + parse_shared_session_inactivity_minutes( + &(SHARED_SESSION_INACTIVITY_MAX_MINUTES).to_string() + ), + Some(SHARED_SESSION_INACTIVITY_MAX_MINUTES), + "the exact max boundary should still be accepted" + ); + assert_eq!( + parse_shared_session_inactivity_minutes( + &(SHARED_SESSION_INACTIVITY_MAX_MINUTES + 1).to_string() + ), + None, + "one past the max boundary must be rejected" + ); + assert_eq!( + parse_shared_session_inactivity_minutes("307445734561825861"), + None, + "a value whose *60 would overflow u64 must be rejected outright" + ); + assert_eq!( + parse_shared_session_inactivity_minutes(&u64::MAX.to_string()), + None + ); +} + +#[test] +fn minutes_rounds_up_and_never_reports_zero() { + assert_eq!( + shared_session_inactivity_minutes(Duration::from_secs(60)), + 1 + ); + assert_eq!( + shared_session_inactivity_minutes(Duration::from_secs(61)), + 2 + ); + assert_eq!( + shared_session_inactivity_minutes(Duration::from_secs(119)), + 2 + ); + assert_eq!(shared_session_inactivity_minutes(Duration::from_secs(0)), 1); +} + +#[test] +fn clamp_revoke_never_exceeds_warning_or_end() { + // Within bounds: unchanged. + assert_eq!(clamp_shared_session_revoke_minutes(5, 25, 30), 5); + // Above warning: pulled down to warning. + assert_eq!(clamp_shared_session_revoke_minutes(50, 25, 30), 25); + // Above end (but below warning is moot since warning < end normally): pulled to the + // smaller of the two neighbors. + assert_eq!(clamp_shared_session_revoke_minutes(50, 60, 30), 30); +} + +#[test] +fn clamp_warning_stays_between_revoke_and_end() { + // Within bounds: unchanged. + assert_eq!(clamp_shared_session_warning_minutes(25, 10, 30), 25); + // Below revoke: pulled up to revoke. + assert_eq!(clamp_shared_session_warning_minutes(5, 10, 30), 10); + // Above end: pulled down to end. + assert_eq!(clamp_shared_session_warning_minutes(50, 10, 30), 30); +} + +#[test] +fn clamp_end_never_falls_below_revoke_or_warning() { + // Within bounds: unchanged. + assert_eq!(clamp_shared_session_end_minutes(30, 10, 25), 30); + // Below warning: pulled up to warning. + assert_eq!(clamp_shared_session_end_minutes(5, 10, 25), 25); + // Below revoke (warning also below revoke here): pulled up to the larger neighbor. + assert_eq!(clamp_shared_session_end_minutes(5, 25, 10), 25); +} + +/// A user can always re-enable a disabled/edge value: clamping never produces a value the +/// user cannot subsequently move away from by editing the same field again. +#[test] +fn clamping_is_idempotent_once_ordering_holds() { + let revoke = clamp_shared_session_revoke_minutes(10, 25, 30); + let warning = clamp_shared_session_warning_minutes(25, revoke, 30); + let end = clamp_shared_session_end_minutes(30, revoke, warning); + assert!(revoke <= warning); + assert!(warning <= end); + + // Re-clamping already-consistent values must not change them further. + assert_eq!( + clamp_shared_session_revoke_minutes(revoke, warning, end), + revoke + ); + assert_eq!( + clamp_shared_session_warning_minutes(warning, revoke, end), + warning + ); + assert_eq!(clamp_shared_session_end_minutes(end, revoke, warning), end); +} diff --git a/app/src/settings_view/mod_tests.rs b/app/src/settings_view/mod_tests.rs index 0bf777a7f59..2920b06d83e 100644 --- a/app/src/settings_view/mod_tests.rs +++ b/app/src/settings_view/mod_tests.rs @@ -1263,6 +1263,71 @@ fn reapply_handles_multi_word_and_case() { }); } +// ── Shared-session inactivity widget split (APP-5313 review finding 4) ────── +// Verifies that the three inactivity-duration rows are registered as three separate +// SettingsWidgets with row-scoped search terms (not one widget with a shared blob), so a +// term unique to one row filters out the other two. Uses StubWidgets with the exact +// search_terms assigned to SharedSessionRevokeEditAccessWidget / SharedSessionWarningWidget / +// SharedSessionEndSessionWidget in features_page.rs. + +fn shared_session_inactivity_stub_page() -> PageType { + let widgets: Vec>> = vec![ + Box::new(StubWidget { + terms: "shared session sharing remote control inactivity idle timeout revoke edit access read-only", + }), + Box::new(StubWidget { + terms: "shared session sharing remote control inactivity idle timeout warn warning ending", + }), + Box::new(StubWidget { + terms: "shared session sharing remote control inactivity idle timeout end disconnect", + }), + ]; + PageType::new_uncategorized(widgets, None) +} + +#[test] +fn shared_session_inactivity_rows_are_independently_filterable() { + App::test((), |mut app| async move { + app.update(|ctx| { + // A term unique to the revoke row. + let mut page = shared_session_inactivity_stub_page(); + page.update_filter("revoke", ctx); + assert_eq!( + visible_widget_count(&page), + 1, + "searching 'revoke' should show only the revoke-edit-access row" + ); + + // A term unique to the warning row. + let mut page = shared_session_inactivity_stub_page(); + page.update_filter("warn", ctx); + assert_eq!( + visible_widget_count(&page), + 1, + "searching 'warn' should show only the warning row" + ); + + // A term unique to the end-session row. + let mut page = shared_session_inactivity_stub_page(); + page.update_filter("disconnect", ctx); + assert_eq!( + visible_widget_count(&page), + 1, + "searching 'disconnect' should show only the end-session row" + ); + + // A term shared by all three still matches all three. + let mut page = shared_session_inactivity_stub_page(); + page.update_filter("inactivity", ctx); + assert_eq!( + visible_widget_count(&page), + 3, + "a shared term should still match every row" + ); + }); + }); +} + #[test] fn empty_query_after_reapply_shows_all_widgets() { // When the search is cleared, the subpage shows all widgets again. diff --git a/app/src/terminal/shared_session/settings.rs b/app/src/terminal/shared_session/settings.rs index 471c8b5189c..66a2158310a 100644 --- a/app/src/terminal/shared_session/settings.rs +++ b/app/src/terminal/shared_session/settings.rs @@ -1,7 +1,13 @@ use std::time::Duration; use settings::macros::define_settings_group; +use settings::manager::SettingsManager; use settings::{RespectUserSyncSetting, Setting, SupportedPlatforms, SyncToCloud}; +use warp_core::user_preferences::GetUserPreferences as _; +use warp_errors::{report_error, report_if_error}; +use warpui::{AppContext, ModelHandle, SingletonEntity}; + +use crate::features::FeatureFlag; define_settings_group!(SharedSessionSettings, settings: [ onboarding_block_shown: SessionSharingOnboardingBlockShown { @@ -58,14 +64,187 @@ define_settings_group!(SharedSessionSettings, settings: [ impl SharedSessionSettings { /// Returns time between showing the inactivity warning modal and ending the session. + /// + /// Uses `saturating_sub` as defense-in-depth: `register_and_enforce_inactivity_ordering` + /// keeps these durations in `revoke <= warn <= end` order at every point they become + /// authoritative (initial load, cloud sync, disk hot-reload), but a plain `Duration` + /// subtraction still panics on underflow if that invariant is ever violated by some + /// path this doesn't cover. pub fn inactivity_period_between_warning_and_ending_session(&self) -> Duration { - *self.inactivity_period_before_ending_session.value() - - *self.inactivity_period_before_warning.value() + self.inactivity_period_before_ending_session + .value() + .saturating_sub(*self.inactivity_period_before_warning.value()) } /// Returns time between revoking roles and showing the inactivity warning modal. + /// + /// See [`Self::inactivity_period_between_warning_and_ending_session`] for why this + /// uses `saturating_sub`. pub fn inactivity_period_between_revoking_roles_and_warning(&self) -> Duration { - *self.inactivity_period_before_warning.value() - - *self.inactivity_period_before_revoking_roles.value() + self.inactivity_period_before_warning + .value() + .saturating_sub(*self.inactivity_period_before_revoking_roles.value()) + } + + /// Registers this settings group, migrates any legacy private-store values for the + /// inactivity durations (see [`migrate_legacy_private_inactivity_settings`]), and keeps + /// those durations in a valid `revoke <= warn <= end` order no matter how they change: + /// at startup (including a hand-edited settings file), via cloud sync, and via disk + /// hot-reload. + /// + /// This ordering is required by the sharer inactivity ladder in + /// `app/src/terminal/view/shared_session/view_impl.rs`, which derives the time between + /// phases via `Duration` subtraction and would otherwise be handed an inconsistent + /// triple whenever these settings are loaded or synced out of order (a plain settings + /// UI edit is already clamped in `app/src/settings_view/features_page.rs`, but that + /// clamp doesn't cover these other paths). + pub fn register_and_enforce_inactivity_ordering(ctx: &mut AppContext) -> ModelHandle { + let handle = Self::register(ctx); + + // Runs after `register()` so the SettingsManager already has the update functions + // for these storage keys (`update_setting_with_storage_key` requires it). + migrate_legacy_private_inactivity_settings(ctx); + Self::enforce_inactivity_ordering(&handle, ctx); + + ctx.subscribe_to_model(&handle, |settings_handle, event, ctx| { + if matches!( + event, + SharedSessionSettingsChangedEvent::InactivityPeriodBeforeRevokingRoles { .. } + | SharedSessionSettingsChangedEvent::InactivityPeriodBeforeWarning { .. } + | SharedSessionSettingsChangedEvent::InactivityPeriodBeforeEndingSession { .. } + ) { + Self::enforce_inactivity_ordering(&settings_handle, ctx); + } + }); + + handle + } + + /// Whether `earlier` is allowed to occur at or before `later` in the inactivity ladder. + /// + /// This is currently a plain numeric comparison. If a duration of zero is later used to + /// mean "this phase is disabled" (a proposed APP-5313 follow-up), this is the one place + /// that needs to change: a disabled (zero) phase should be exempt from the comparison + /// rather than treated as the smallest legal duration. + fn ladder_phase_order_ok(earlier: Duration, later: Duration) -> bool { + earlier <= later + } + + /// Corrects the inactivity durations in place if they violate the required + /// `revoke <= warn <= end` ordering, clamping an out-of-order value up to its earlier + /// neighbor rather than rejecting the update outright. + fn enforce_inactivity_ordering(handle: &ModelHandle, ctx: &mut AppContext) { + let (revoke, warn, end) = handle.read(ctx, |settings, _| { + ( + *settings.inactivity_period_before_revoking_roles.value(), + *settings.inactivity_period_before_warning.value(), + *settings.inactivity_period_before_ending_session.value(), + ) + }); + + let corrected_warn = if Self::ladder_phase_order_ok(revoke, warn) { + warn + } else { + revoke + }; + let corrected_end = if Self::ladder_phase_order_ok(corrected_warn, end) { + end + } else { + corrected_warn + }; + + handle.clone().update(ctx, |settings, ctx| { + if corrected_warn != warn { + report_if_error!( + settings + .inactivity_period_before_warning + .set_value(corrected_warn, ctx) + ); + } + if corrected_end != end { + report_if_error!( + settings + .inactivity_period_before_ending_session + .set_value(corrected_end, ctx) + ); + } + }); } } + +/// Key written to the private (platform-native) store once the legacy private values for +/// the inactivity durations below have been migrated into their new public location. +/// +/// These three settings used to be `private: true` (APP-5313); flipping them to public +/// means `new_from_storage` only reads `PublicPreferences`, so without this one-time copy, +/// an existing user's customized values would silently revert to the defaults. This marker +/// is independent of `SETTINGS_FILE_MIGRATION_COMPLETE_KEY` in `app/src/settings/init.rs`, +/// which is already set for existing `SettingsFile` users and would otherwise never revisit +/// these newly-public keys. +const LEGACY_INACTIVITY_SETTINGS_MIGRATED_KEY: &str = + "SharedSessionInactivitySettingsMigratedFromPrivateStore"; + +/// One-time migration: copies each inactivity duration's legacy private-store value into +/// its new public (TOML) location, but only when the public location doesn't already have +/// a value, so it never clobbers a value the user has already set through the new UI or +/// settings file. +fn migrate_legacy_private_inactivity_settings(ctx: &mut AppContext) { + // When the settings file feature is off, public settings fall back to the same private + // store as before, so there's nothing to migrate. + if !FeatureFlag::SettingsFile.is_enabled() { + return; + } + + let already_migrated = ctx + .private_user_preferences() + .read_value(LEGACY_INACTIVITY_SETTINGS_MIGRATED_KEY) + .unwrap_or_default() + .as_deref() + == Some("true"); + if already_migrated { + return; + } + + let keys = [ + InactivityPeriodBeforeRevokingRoles::storage_key(), + InactivityPeriodBeforeWarning::storage_key(), + InactivityPeriodBeforeEndingSession::storage_key(), + ]; + + let values_to_migrate: Vec<(&'static str, String)> = keys + .into_iter() + .filter(|key| { + matches!( + SettingsManager::as_ref(ctx).read_local_setting_value(key, ctx), + Ok(None) + ) + }) + .filter_map(|key| { + let value = ctx + .private_user_preferences() + .read_value(key) + .unwrap_or_default()?; + Some((key, value)) + }) + .collect(); + + SettingsManager::handle(ctx).update(ctx, |manager, ctx| { + for (key, value) in values_to_migrate { + if let Err(err) = manager.update_setting_with_storage_key(key, value, false, ctx) { + report_error!( + err.context(format!("Failed to migrate legacy inactivity setting {key}")) + ); + } + } + }); + + report_if_error!( + ctx.private_user_preferences() + .write_value(LEGACY_INACTIVITY_SETTINGS_MIGRATED_KEY, "true".to_owned()) + .map_err(|err| anyhow::anyhow!(err)) + ); +} + +#[cfg(test)] +#[path = "settings_tests.rs"] +mod settings_tests; diff --git a/app/src/terminal/shared_session/settings_tests.rs b/app/src/terminal/shared_session/settings_tests.rs new file mode 100644 index 00000000000..05ba94af8f6 --- /dev/null +++ b/app/src/terminal/shared_session/settings_tests.rs @@ -0,0 +1,285 @@ +use settings::{PrivatePreferences, PublicPreferences, Setting, SettingsManager}; +use warp_core::features::FeatureFlag; +use warp_core::user_preferences::GetUserPreferences as _; +use warpui::{App, AppContext, SingletonEntity}; +use warpui_extras::user_preferences; + +use super::*; + +fn init_test_app(ctx: &mut AppContext) { + ctx.add_singleton_model(move |_| { + PublicPreferences::new(Box::::default()) + }); + ctx.add_singleton_model(move |_| -> PrivatePreferences { + PrivatePreferences::new(Box::::default()) + }); + ctx.add_singleton_model(|_| SettingsManager::default()); +} + +fn write_public(ctx: &AppContext, key: &str, duration: Duration) { + // Any of the three (now-public) settings routes to the same PublicPreferences backend; + // `preferences_for_setting` is the public API for reaching it from outside the + // `settings` crate. + InactivityPeriodBeforeRevokingRoles::preferences_for_setting(ctx) + .write_value(key, serde_json::to_string(&duration).unwrap()) + .unwrap(); +} + +fn write_private(ctx: &AppContext, key: &str, duration: Duration) { + ctx.private_user_preferences() + .write_value(key, serde_json::to_string(&duration).unwrap()) + .unwrap(); +} + +// --------------------------------------------------------------------------- +// Legacy private -> public migration (review finding 1) +// --------------------------------------------------------------------------- + +#[test] +fn legacy_private_value_survives_migration_even_when_settings_file_marker_already_set() { + App::test((), |mut app| async move { + let _guard = FeatureFlag::SettingsFile.override_enabled(true); + app.update(init_test_app); + + // Simulate a pre-existing user for whom the general native->TOML migration already + // ran and recorded its completion marker, before these three settings became public. + app.update(|ctx| { + ctx.private_user_preferences() + .write_value("SettingsFileMigrationComplete", "true".to_owned()) + .unwrap(); + write_private( + ctx, + InactivityPeriodBeforeRevokingRoles::storage_key(), + Duration::from_secs(900), + ); + }); + + app.update(|ctx| { + SharedSessionSettings::register_and_enforce_inactivity_ordering(ctx); + }); + + app.read(|ctx| { + assert_eq!( + *SharedSessionSettings::as_ref(ctx) + .inactivity_period_before_revoking_roles + .value(), + Duration::from_secs(900), + "legacy private-store value should survive the flip to a public setting" + ); + }); + }); +} + +#[test] +fn migration_does_not_overwrite_already_set_public_value() { + App::test((), |mut app| async move { + let _guard = FeatureFlag::SettingsFile.override_enabled(true); + app.update(init_test_app); + + app.update(|ctx| { + // The user already has an explicit value in the new public location... + write_public( + ctx, + InactivityPeriodBeforeRevokingRoles::storage_key(), + Duration::from_secs(120), + ); + // ...while a stale legacy private-store value also happens to exist. + write_private( + ctx, + InactivityPeriodBeforeRevokingRoles::storage_key(), + Duration::from_secs(999), + ); + }); + + app.update(|ctx| { + SharedSessionSettings::register_and_enforce_inactivity_ordering(ctx); + }); + + app.read(|ctx| { + assert_eq!( + *SharedSessionSettings::as_ref(ctx) + .inactivity_period_before_revoking_roles + .value(), + Duration::from_secs(120), + "migration must not clobber a value already explicitly set in the public location" + ); + }); + }); +} + +#[test] +fn migration_is_one_time_via_its_own_marker() { + App::test((), |mut app| async move { + let _guard = FeatureFlag::SettingsFile.override_enabled(true); + app.update(init_test_app); + + app.update(|ctx| { + write_private( + ctx, + InactivityPeriodBeforeRevokingRoles::storage_key(), + Duration::from_secs(900), + ); + }); + + // Register once, then run the migration explicitly (simulating a launch with a + // pre-existing private-store value). + app.update(|ctx| { + SharedSessionSettings::register(ctx); + }); + app.update(migrate_legacy_private_inactivity_settings); + + app.read(|ctx| { + assert_eq!( + ctx.private_user_preferences() + .read_value(LEGACY_INACTIVITY_SETTINGS_MIGRATED_KEY) + .unwrap() + .as_deref(), + Some("true"), + "migration should record its own completion marker" + ); + assert_eq!( + *SharedSessionSettings::as_ref(ctx) + .inactivity_period_before_revoking_roles + .value(), + Duration::from_secs(900) + ); + }); + + // The user then explicitly clears the migrated value (e.g. removing it from their + // settings file / resetting to default). + app.update(|ctx| { + SharedSessionSettings::handle(ctx).update(ctx, |settings, ctx| { + settings + .inactivity_period_before_revoking_roles + .clear_value(ctx) + .unwrap(); + }); + }); + + // Running the migration again (simulating a second launch) must be a no-op: its own + // marker is already set, so it must not re-copy the stale legacy value and clobber + // the user's explicit reset. + app.update(migrate_legacy_private_inactivity_settings); + + app.read(|ctx| { + assert_eq!( + *SharedSessionSettings::as_ref(ctx) + .inactivity_period_before_revoking_roles + .value(), + InactivityPeriodBeforeRevokingRoles::default_value(), + "migration must not re-run once its own marker is set" + ); + }); + }); +} + +// --------------------------------------------------------------------------- +// Ordering enforcement at the authoritative boundary (review finding 2) +// --------------------------------------------------------------------------- + +#[test] +fn register_corrects_out_of_order_values_from_storage() { + App::test((), |mut app| async move { + let _guard = FeatureFlag::SettingsFile.override_enabled(true); + app.update(init_test_app); + + // Simulate a hand-edited settings file with revoke > warn. + app.update(|ctx| { + write_public( + ctx, + InactivityPeriodBeforeRevokingRoles::storage_key(), + Duration::from_secs(1000), + ); + write_public( + ctx, + InactivityPeriodBeforeWarning::storage_key(), + Duration::from_secs(500), + ); + }); + + app.update(|ctx| { + SharedSessionSettings::register_and_enforce_inactivity_ordering(ctx); + }); + + app.read(|ctx| { + let settings = SharedSessionSettings::as_ref(ctx); + let revoke = *settings.inactivity_period_before_revoking_roles.value(); + let warn = *settings.inactivity_period_before_warning.value(); + let end = *settings.inactivity_period_before_ending_session.value(); + assert!( + revoke <= warn && warn <= end, + "ordering must hold after loading an inconsistent file: \ + revoke={revoke:?} warn={warn:?} end={end:?}" + ); + assert_eq!(warn, revoke, "warn should be pulled up to revoke's value"); + }); + }); +} + +#[test] +fn cloud_sync_update_producing_bad_ordering_gets_corrected() { + App::test((), |mut app| async move { + let _guard = FeatureFlag::SettingsFile.override_enabled(true); + app.update(init_test_app); + + app.update(|ctx| { + SharedSessionSettings::register_and_enforce_inactivity_ordering(ctx); + }); + + // A cloud-synced update sets `end` below the current `warn` (defaults: revoke=600s, + // warn=1500s, end=1800s). + app.update(|ctx| { + SharedSessionSettings::handle(ctx).update(ctx, |settings, ctx| { + settings + .inactivity_period_before_ending_session + .set_value_from_cloud_sync(Duration::from_secs(100), ctx) + .unwrap(); + }); + }); + + app.read(|ctx| { + let settings = SharedSessionSettings::as_ref(ctx); + let revoke = *settings.inactivity_period_before_revoking_roles.value(); + let warn = *settings.inactivity_period_before_warning.value(); + let end = *settings.inactivity_period_before_ending_session.value(); + assert!( + revoke <= warn && warn <= end, + "ordering must hold after a bad cloud sync update: \ + revoke={revoke:?} warn={warn:?} end={end:?}" + ); + assert_eq!( + end, warn, + "end should be pulled back up to warn's value rather than left below it" + ); + }); + }); +} + +#[test] +fn derived_intervals_never_panic_on_out_of_order_values() { + // Directly construct an inconsistent group (bypassing the ordering enforcement entirely) + // to prove the derived-interval helpers are defensive regardless of how a bad ordering + // arises, not just against the paths this change actively guards. + let settings = SharedSessionSettings { + onboarding_block_shown: SessionSharingOnboardingBlockShown::new(None), + inactivity_period_before_ending_session: InactivityPeriodBeforeEndingSession::new(Some( + Duration::from_secs(10), + )), + inactivity_period_before_warning: InactivityPeriodBeforeWarning::new(Some( + Duration::from_secs(500), + )), + inactivity_period_before_revoking_roles: InactivityPeriodBeforeRevokingRoles::new(Some( + Duration::from_secs(600), + )), + viewer_driven_sizing_enabled: ViewerDrivenSizingEnabled::new(None), + }; + + assert_eq!( + settings.inactivity_period_between_warning_and_ending_session(), + Duration::ZERO + ); + assert_eq!( + settings.inactivity_period_between_revoking_roles_and_warning(), + Duration::ZERO + ); +} diff --git a/app/src/test_util/settings.rs b/app/src/test_util/settings.rs index 078af0a0d7b..d20709bd9d4 100644 --- a/app/src/test_util/settings.rs +++ b/app/src/test_util/settings.rs @@ -119,7 +119,7 @@ pub fn initialize_settings_for_tests_with_mode( SharedObjectLimitBannerSettings::register(app); WarpDriveSettings::register(app); WindowSettings::register(app); - SharedSessionSettings::register(app); + app.update(SharedSessionSettings::register_and_enforce_inactivity_ordering); CodeSettings::register(app); SemanticSelection::register(app);