diff --git a/app/src/settings_view/mcp_servers/installation_modal.rs b/app/src/settings_view/mcp_servers/installation_modal.rs index bdd3be92f0e..f123b3ae3ec 100644 --- a/app/src/settings_view/mcp_servers/installation_modal.rs +++ b/app/src/settings_view/mcp_servers/installation_modal.rs @@ -13,8 +13,8 @@ use warpui::keymap::Keystroke; use warpui::platform::Cursor; use warpui::ui_components::components::{UiComponent, UiComponentStyles}; use warpui::{ - AppContext, Element, Entity, FocusContext, SingletonEntity, TypedActionView, View, ViewContext, - ViewHandle, + AppContext, Element, Entity, EntityId, FocusContext, SingletonEntity, TypedActionView, View, + ViewContext, ViewHandle, }; use crate::ai::mcp::templatable_installation::{VariableType, VariableValue}; @@ -486,6 +486,27 @@ impl View for InstallationModalBody { "MCPTemplateInstallationModalBody" } + fn child_view_ids(&self, _app: &AppContext) -> Vec { + // `variable_inputs`' `TextInput` editors are created via plain + // `ctx.add_view` (no structural parent edge), and this view only + // renders them while `templatable_mcp_server` is set (i.e. while an + // install is pending), not while the modal is closed (see + // `MCPServersSettingsPageView::get_modal_content`). On Cancel the + // pending server/inputs are left populated (only cleared on a + // completed Install), so a cross-window tab drag could otherwise + // orphan these editors exactly like `AboutPageView` / + // `BillingAndUsageDispatchView` did for `SettingsView` (APP-5314). + // Declaring them here keeps them in the transferable subtree + // regardless of the modal's open/closed state. + self.variable_inputs + .values() + .map(|input| match input { + VariableInput::TextInput(handle) => handle.id(), + VariableInput::Dropdown { handle, .. } => handle.id(), + }) + .collect() + } + fn on_focus(&mut self, focus_ctx: &FocusContext, ctx: &mut ViewContext) { if focus_ctx.is_self_focused() { // Focus the first text input editor, if any. @@ -574,3 +595,7 @@ impl TypedActionView for InstallationModalBody { } } } + +#[cfg(test)] +#[path = "installation_modal_tests.rs"] +mod tests; diff --git a/app/src/settings_view/mcp_servers/installation_modal_tests.rs b/app/src/settings_view/mcp_servers/installation_modal_tests.rs new file mode 100644 index 00000000000..a5a773b47b6 --- /dev/null +++ b/app/src/settings_view/mcp_servers/installation_modal_tests.rs @@ -0,0 +1,119 @@ +use uuid::Uuid; +use warpui::elements::Empty; +use warpui::platform::WindowStyle; +use warpui::{App, AppContext, Element, Entity, TypedActionView, View}; + +use super::*; +use crate::ai::mcp::{JsonTemplate, TemplatableMCPServer, TemplateVariable}; +use crate::appearance::Appearance; + +#[derive(Default)] +struct TestRoot; + +impl Entity for TestRoot { + type Event = (); +} + +impl View for TestRoot { + fn ui_name() -> &'static str { + "TestRoot" + } + + fn render(&self, _: &AppContext) -> Box { + Empty::new().finish() + } +} + +impl TypedActionView for TestRoot { + type Action = (); +} + +#[test] +fn child_view_ids_covers_text_input_and_dropdown_variables() { + // Regression test mirroring APP-5314: `InstallationModalBody` creates its + // free-text `TextInput` editors via plain `ctx.add_view` (no structural + // parent edge), and only renders them while an install is pending (see + // `MCPServersSettingsPageView::get_modal_content`). On Cancel, the + // pending server/inputs are left populated (only cleared on a completed + // Install), so without `child_view_ids` a cross-window tab drag could + // orphan these editors in the source window exactly like `AboutPageView` + // did for `SettingsView`. + App::test((), |mut app| async move { + crate::test_util::settings::initialize_settings_for_tests(&mut app); + app.add_singleton_model(|_| crate::server::server_api::ServerApiProvider::new_for_test()); + app.add_singleton_model(|_| crate::auth::AuthStateProvider::new_for_test()); + app.add_singleton_model(|_| Appearance::mock()); + app.add_singleton_model(crate::cloud_object::model::persistence::CloudModel::mock); + app.add_singleton_model(crate::workspaces::user_workspaces::UserWorkspaces::default_mock); + app.add_singleton_model(crate::settings::PrivacySettings::mock); + app.add_singleton_model(|_| crate::network::NetworkStatus::new()); + app.add_singleton_model(crate::workspaces::team_tester::TeamTesterStatus::mock); + app.add_singleton_model(crate::server::sync_queue::SyncQueue::mock); + app.add_singleton_model(crate::server::cloud_objects::update_manager::UpdateManager::mock); + app.add_singleton_model(|_| { + crate::settings_view::keybindings::KeybindingChangedNotifier::new() + }); + app.add_singleton_model(|_| { + crate::ai::ambient_agents::github_auth_notifier::GitHubAuthNotifier::new() + }); + app.add_singleton_model(|_| TemplatableMCPServerManager::default()); + + let (window_id, _) = app.add_window(WindowStyle::NotStealFocus, |_| TestRoot); + + let body = app.add_typed_action_view(window_id, InstallationModalBody::new); + + // One freetext variable (produces a `TextInput`) and one with + // allowed values (produces a `Dropdown`), so the test covers both + // `VariableInput` arms. + let server = TemplatableMCPServer { + uuid: Uuid::new_v4(), + template: JsonTemplate { + json: "{}".to_string(), + variables: vec![ + TemplateVariable { + key: "api_key".to_string(), + allowed_values: None, + }, + TemplateVariable { + key: "region".to_string(), + allowed_values: Some(vec!["us".to_string(), "eu".to_string()]), + }, + ], + }, + ..Default::default() + }; + + body.update(&mut app, |body, ctx| { + body.set_templatable_mcp_server(Some(server), None, ctx); + }); + + let variable_input_ids: Vec<_> = body.read(&app, |body, _| { + body.variable_inputs + .values() + .map(|input| match input { + VariableInput::TextInput(handle) => handle.id(), + VariableInput::Dropdown { handle, .. } => handle.id(), + }) + .collect() + }); + assert_eq!( + variable_input_ids.len(), + 2, + "sanity check: both variables should have produced an input widget" + ); + + let child_view_ids = body.read(&app, |body, ctx| body.child_view_ids(ctx)); + + assert_eq!( + child_view_ids.len(), + variable_input_ids.len(), + "child_view_ids must cover every variable input widget" + ); + for id in variable_input_ids { + assert!( + child_view_ids.contains(&id), + "child_view_ids is missing variable input {id:?}" + ); + } + }); +} diff --git a/app/src/settings_view/mod.rs b/app/src/settings_view/mod.rs index 44da475d969..14be46725a8 100644 --- a/app/src/settings_view/mod.rs +++ b/app/src/settings_view/mod.rs @@ -44,7 +44,7 @@ use warpui::elements::{ use warpui::fonts::{Properties, Weight}; use warpui::keymap::{ContextPredicate, EnabledPredicate, FixedBinding}; use warpui::{ - Action, AppContext, Element, Entity, ModelHandle, SingletonEntity, TypedActionView, + Action, AppContext, Element, Entity, EntityId, ModelHandle, SingletonEntity, TypedActionView, UpdateView as _, View, ViewContext, ViewHandle, id, }; @@ -1495,6 +1495,45 @@ impl SettingsView { }) } + /// Computes the ids of every view `SettingsView` directly owns: every + /// page's handle plus `search_editor` and `context_menu`. This is the + /// literal body of `View::child_view_ids` below, split out into a + /// standalone function of plain inputs (rather than `&self`) so it can be + /// exercised directly in a unit test against real `SettingsPage`/ + /// `SettingsPageViewHandle` values, without needing to construct a full + /// `SettingsView` (whose `new` pulls in singleton models for ~18 pages + /// spanning billing, teams, warp drive, and MCP servers). See + /// `settings_view_child_view_ids_covers_pages_and_own_handles` in + /// `mod_tests.rs`. + /// + /// `SettingsView` owns every settings page, but `render` only ever embeds + /// the currently active one (see `filtered_pages` above), so inactive + /// pages are invisible to the render-time parent graph. Most pages are + /// created with `add_typed_action_view`, which records a structural + /// parent edge, but a couple (e.g. `AboutPageView`, + /// `BillingAndUsageDispatchView`) use plain `ctx.add_view` and have no + /// such edge. Without this, a cross-window tab drag can leave those pages + /// orphaned in the source window, and the destination window later + /// panics trying to render them (see APP-5314). + /// + /// `settings_pages` is the single source of truth for the page list, so + /// iterating it here (via the exhaustive `SettingsPageViewHandle::view_id` + /// match) keeps this self-maintaining: a newly added page is covered + /// automatically, with no separate list to remember to update. + fn owned_view_ids( + settings_pages: &[SettingsPage], + search_editor: &ViewHandle, + context_menu: &ViewHandle>, + ) -> Vec { + let mut ids: Vec = settings_pages + .iter() + .map(|page| page.view_handle.view_id()) + .collect(); + ids.push(search_editor.id()); + ids.push(context_menu.id()); + ids + } + fn handle_search_editor_event( &mut self, editor: ViewHandle, @@ -2509,6 +2548,14 @@ impl View for SettingsView { "SettingsViewInTab" } + fn child_view_ids(&self, _app: &AppContext) -> Vec { + Self::owned_view_ids( + &self.settings_pages, + &self.search_editor, + &self.context_menu, + ) + } + fn render(&self, app: &AppContext) -> Box { let settings_pages = self.filtered_pages(app).collect_vec(); let appearance = Appearance::as_ref(app); diff --git a/app/src/settings_view/mod_tests.rs b/app/src/settings_view/mod_tests.rs index 0bf777a7f59..31c09e44477 100644 --- a/app/src/settings_view/mod_tests.rs +++ b/app/src/settings_view/mod_tests.rs @@ -1,6 +1,7 @@ use settings_page::{FilteredPageType, MatchData, PageType, SettingsWidget, search_terms_match}; use warpui::elements::Empty; -use warpui::{App, AppContext, Element, Entity, View}; +use warpui::platform::WindowStyle; +use warpui::{App, AppContext, Element, Entity, TypedActionView, View, ViewHandle}; use super::*; use crate::appearance::Appearance; @@ -1282,3 +1283,108 @@ fn empty_query_after_reapply_shows_all_widgets() { }); }); } + +// ── SettingsView::child_view_ids coverage (APP-5314) ──────────────────────── +// Regression test for the Settings cross-window drag crash: `SettingsView` +// must report every owned page (and its other directly-held handles) via +// `child_view_ids`, or a page that was never made active can be orphaned by +// `transfer_view_tree_to_window` and later panic when the destination window +// tries to render it. +// +// This exercises `SettingsView::owned_view_ids` directly — the exact +// function `View::child_view_ids` delegates to (see `mod.rs`) — against +// real `SettingsPage`/`SettingsPageViewHandle` values built from the real +// `AboutPageView`, rather than against a synthetic stand-in. Constructing a +// full `SettingsView` here isn't practical: `SettingsView::new` pulls in +// singleton models for ~18 pages spanning billing, teams, warp drive, +// referrals, and MCP servers (some of which kick off live async server +// calls), which would make the test slow and flaky rather than a reliable +// unit test. +#[derive(Default)] +struct ChildViewIdsTestRoot; + +impl Entity for ChildViewIdsTestRoot { + type Event = (); +} + +impl View for ChildViewIdsTestRoot { + fn ui_name() -> &'static str { + "ChildViewIdsTestRoot" + } + + fn render(&self, _: &AppContext) -> Box { + Empty::new().finish() + } +} + +impl TypedActionView for ChildViewIdsTestRoot { + type Action = (); +} + +#[test] +fn settings_view_owned_view_ids_covers_pages_and_own_handles() { + App::test((), |mut app| async move { + crate::test_util::settings::initialize_settings_for_tests(&mut app); + // Mirrors `environments_page_tests::init_env_page_view_test_models`: + // most Settings page views assume these singleton models exist. + app.add_singleton_model(|_| crate::server::server_api::ServerApiProvider::new_for_test()); + app.add_singleton_model(|_| crate::auth::AuthStateProvider::new_for_test()); + app.add_singleton_model(|_| Appearance::mock()); + app.add_singleton_model(crate::cloud_object::model::persistence::CloudModel::mock); + app.add_singleton_model(crate::workspaces::user_workspaces::UserWorkspaces::default_mock); + app.add_singleton_model(crate::settings::PrivacySettings::mock); + app.add_singleton_model(|_| crate::network::NetworkStatus::new()); + app.add_singleton_model(crate::workspaces::team_tester::TeamTesterStatus::mock); + app.add_singleton_model(crate::server::sync_queue::SyncQueue::mock); + app.add_singleton_model(crate::server::cloud_objects::update_manager::UpdateManager::mock); + app.add_singleton_model(|_| { + crate::settings_view::keybindings::KeybindingChangedNotifier::new() + }); + app.add_singleton_model(|_| { + crate::ai::ambient_agents::github_auth_notifier::GitHubAuthNotifier::new() + }); + + let (window_id, _) = app.add_window(WindowStyle::NotStealFocus, |_| ChildViewIdsTestRoot); + + // Two real pages. Both happen to be `SettingsPageViewHandle::About` + // since `AboutPageView` is the only settings page cheap enough to + // construct without mocking a large dependency graph (see comment + // above). What's under test is the mapping over `settings_pages`, + // not per-variant coverage: every arm of the exhaustive + // `SettingsPageViewHandle::view_id` match is structurally identical + // (`Variant(handle) => handle.id()`), and the compiler enforces that + // no variant is missing from that match. + let about_1: ViewHandle = app.add_view(window_id, AboutPageView::new); + let about_2: ViewHandle = app.add_view(window_id, AboutPageView::new); + let about_1_id = about_1.id(); + let about_2_id = about_2.id(); + let settings_pages = vec![SettingsPage::new(about_1), SettingsPage::new(about_2)]; + + let font_family = app.update(|ctx| Appearance::as_ref(ctx).ui_font_family()); + let search_editor = app.add_typed_action_view(window_id, |ctx| { + EditorView::single_line( + SingleLineEditorOptions { + text: TextOptions { + font_family_override: Some(font_family), + ..Default::default() + }, + ..Default::default() + }, + ctx, + ) + }); + let search_editor_id = search_editor.id(); + + let context_menu: ViewHandle> = + app.add_typed_action_view(window_id, |_| Menu::new()); + let context_menu_id = context_menu.id(); + + let ids = SettingsView::owned_view_ids(&settings_pages, &search_editor, &context_menu); + + assert_eq!( + ids, + vec![about_1_id, about_2_id, search_editor_id, context_menu_id], + "child_view_ids must cover every page plus search_editor and context_menu" + ); + }); +} diff --git a/app/src/settings_view/settings_page.rs b/app/src/settings_view/settings_page.rs index e55abd2420d..36bb5f7d9e0 100644 --- a/app/src/settings_view/settings_page.rs +++ b/app/src/settings_view/settings_page.rs @@ -24,7 +24,7 @@ use warpui::platform::Cursor; use warpui::ui_components::button::{Button, ButtonVariant}; use warpui::ui_components::components::{Coords, UiComponent, UiComponentStyles}; use warpui::units::Pixels; -use warpui::{Action, AppContext, SingletonEntity, ViewContext, ViewHandle}; +use warpui::{Action, AppContext, EntityId, SingletonEntity, ViewContext, ViewHandle}; use super::SettingsSection; use super::about_page::AboutPageView; @@ -146,6 +146,38 @@ impl SettingsPageViewHandle { WarpDrive(view_handle) => ChildView::new(view_handle).finish(), } } + + /// Returns the entity id of the wrapped page view handle. + /// + /// This match is intentionally exhaustive (no wildcard arm) so that + /// adding a new [`SettingsPageViewHandle`] variant forces a compile + /// error here, the same way it already does in [`Self::child_view`]. + /// `SettingsView::child_view_ids` builds its list by iterating + /// `settings_pages` and calling this method, so every page is covered + /// automatically without a separate hand-maintained id list. + pub fn view_id(&self) -> EntityId { + use SettingsPageViewHandle::*; + match self { + Main(view_handle) => view_handle.id(), + Appearance(view_handle) => view_handle.id(), + Features(view_handle) => view_handle.id(), + SharedBlocks(view_handle) => view_handle.id(), + Keybindings(view_handle) => view_handle.id(), + About(view_handle) => view_handle.id(), + Code(view_handle) => view_handle.id(), + Teams(view_handle) => view_handle.id(), + OzCloudAPIKeys(view_handle) => view_handle.id(), + Privacy(view_handle) => view_handle.id(), + Warpify(view_handle) => view_handle.id(), + Referrals(view_handle) => view_handle.id(), + Scripting(view_handle) => view_handle.id(), + AI(view_handle) => view_handle.id(), + CloudEnvironments(view_handle) => view_handle.id(), + BillingAndUsage(view_handle) => view_handle.id(), + MCPServers(view_handle) => view_handle.id(), + WarpDrive(view_handle) => view_handle.id(), + } + } } impl From> for SettingsPageViewHandle { diff --git a/crates/warpui_core/src/core/transfer_view_tests.rs b/crates/warpui_core/src/core/transfer_view_tests.rs index 12496f3cf20..7adcdcbac12 100644 --- a/crates/warpui_core/src/core/transfer_view_tests.rs +++ b/crates/warpui_core/src/core/transfer_view_tests.rs @@ -1062,3 +1062,179 @@ fn test_transfer_view_tree_reconciles_views_known_only_to_target_presenter() { }); }); } + +#[test] +fn test_transfer_view_tree_moves_add_view_pages_never_made_active_when_declared_via_child_view_ids() +{ + // NOTE: this is a transfer-machinery test, not an APP-5314 regression + // test. It exercises a synthetic `SettingsViewLike` type that supplies + // its own `child_view_ids` override, mirroring the *shape* of the real + // `app::settings_view::SettingsView` bug (a view that renders only its + // active child, with a mix of structural `add_typed_action_view` + // children and plain `add_view` children) — but it does not call, and + // therefore cannot catch a regression in, the real + // `SettingsView::child_view_ids`. The actual APP-5314 regression + // coverage lives in `app/src/settings_view/mod_tests.rs` + // (`settings_view_owned_view_ids_covers_pages_and_own_handles`), which + // exercises `SettingsView::owned_view_ids` directly against real + // `SettingsPage`/`SettingsPageViewHandle` values. This test is kept + // because it's still a valid, generic check that + // `transfer_view_tree_to_window` honors `View::child_view_ids` for + // views whose owned children are a mix of structural and non-structural + // (never-rendered) handles — the general mechanism the real fix relies + // on — not because it protects `SettingsView` itself. + // + // `SettingsView` owns every settings page, but `SettingsView::render` + // (via `filtered_pages`) only ever embeds the currently *active* page, so + // inactive pages are invisible to the render-time parent graph. Most + // pages are created with `ctx.add_typed_action_view` (which records a + // structural parent edge), but `AboutPageView` and + // `BillingAndUsageDispatchView` are created with plain `ctx.add_view` and + // get no such edge. Before `SettingsView::child_view_ids` was added to + // report every owned page, a cross-window tab drag left those two pages + // orphaned in the source window, and the destination window later + // panicked trying to render them ("circular view reference", i.e. the + // missing-view branch of `AppContext::view`). + struct StructuralPage; + + impl Entity for StructuralPage { + type Event = (); + } + + impl View for StructuralPage { + fn render(&self, _: &AppContext) -> Box { + Empty::new().finish() + } + + fn ui_name() -> &'static str { + "StructuralPage" + } + } + + impl TypedActionView for StructuralPage { + type Action = (); + } + + // Stands in for `AboutPageView` / `BillingAndUsageDispatchView`: created + // via plain `ctx.add_view`, so it gets no structural parent edge. + struct AddViewOnlyPage; + + impl Entity for AddViewOnlyPage { + type Event = (); + } + + impl View for AddViewOnlyPage { + fn render(&self, _: &AppContext) -> Box { + Empty::new().finish() + } + + fn ui_name() -> &'static str { + "AddViewOnlyPage" + } + } + + struct SettingsViewLike { + // Every owned page is kept alive here, mirroring how the real + // `SettingsView` retains each page's `ViewHandle` inside its + // `settings_pages: Vec` field. If a handle were instead + // dropped after construction (keeping only its id), the view would be + // removed by the framework's ref-counted cleanup before any transfer + // ever ran, which would defeat the point of this test. + structural_pages: Vec>, + add_view_pages: Vec>, + } + + impl Entity for SettingsViewLike { + type Event = (); + } + + impl View for SettingsViewLike { + fn render(&self, _: &AppContext) -> Box { + // Only the active (first) page is ever embedded, exactly like + // `SettingsView::render` -> `filtered_pages`. + ChildView::new(&self.structural_pages[0]).finish() + } + + fn ui_name() -> &'static str { + "SettingsViewLike" + } + + fn child_view_ids(&self, _app: &AppContext) -> Vec { + self.structural_pages + .iter() + .map(|handle| handle.id()) + .chain(self.add_view_pages.iter().map(|handle| handle.id())) + .collect() + } + } + + impl TypedActionView for SettingsViewLike { + type Action = (); + } + + App::test((), |mut app| async move { + let (window_1_id, _) = app.add_window(WindowStyle::NotStealFocus, |_| StructuralPage); + let (window_2_id, _) = app.add_window(WindowStyle::NotStealFocus, |_| StructuralPage); + + let mut about_id = None; + let mut billing_id = None; + let settings = app.add_typed_action_view(window_1_id, |ctx| { + // Structural (typed-action) pages, like MainSettingsPageView, AI + // SettingsPageView, etc: these already survive a transfer via the + // structural parent/child graph, with or without our fix. + let active = ctx.add_typed_action_view(|_| StructuralPage); + let other_structural = ctx.add_typed_action_view(|_| StructuralPage); + + // Plain `add_view` pages, like AboutPageView and + // BillingAndUsageDispatchView: never active, never rendered, and + // (before the fix) not reachable from the parent at all. + let about = ctx.add_view(|_| AddViewOnlyPage); + let billing_and_usage = ctx.add_view(|_| AddViewOnlyPage); + about_id = Some(about.id()); + billing_id = Some(billing_and_usage.id()); + + SettingsViewLike { + structural_pages: vec![active, other_structural], + add_view_pages: vec![about, billing_and_usage], + } + }); + let settings_id = settings.id(); + let about_id = about_id.expect("about page should have been created"); + let billing_id = billing_id.expect("billing page should have been created"); + + let transferred = app + .update(|ctx| ctx.transfer_view_tree_to_window(settings_id, window_1_id, window_2_id)); + + assert!( + transferred.contains(&settings_id), + "SettingsView-like root should be transferred" + ); + assert!( + transferred.contains(&about_id), + "AboutPageView-like page should be transferred even though it's never active" + ); + assert!( + transferred.contains(&billing_id), + "BillingAndUsageDispatchView-like page should be transferred even though it's never active" + ); + + app.read(|ctx| { + assert!( + ctx.windows[&window_2_id].views.contains_key(&about_id), + "About page should now live in window 2, not be orphaned in window 1" + ); + assert!( + !ctx.windows[&window_1_id].views.contains_key(&about_id), + "About page should no longer be in window 1" + ); + assert!( + ctx.windows[&window_2_id].views.contains_key(&billing_id), + "Billing and usage page should now live in window 2, not be orphaned in window 1" + ); + assert!( + !ctx.windows[&window_1_id].views.contains_key(&billing_id), + "Billing and usage page should no longer be in window 1" + ); + }); + }); +}