Fix Settings crash on cross-window tab drag (APP-5314)

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<EntityId> {
+        // `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<Self>) {
         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;

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<dyn Element> {
+        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:?}"
+            );
+        }
+    });
+}

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<EditorView>,
+        context_menu: &ViewHandle<Menu<SettingsAction>>,
+    ) -> Vec<EntityId> {
+        let mut ids: Vec<EntityId> = 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<EditorView>,
@@ -2509,6 +2548,14 @@ impl View for SettingsView {
         "SettingsViewInTab"
     }
 
+    fn child_view_ids(&self, _app: &AppContext) -> Vec<EntityId> {
+        Self::owned_view_ids(
+            &self.settings_pages,
+            &self.search_editor,
+            &self.context_menu,
+        )
+    }
+
     fn render(&self, app: &AppContext) -> Box<dyn Element> {
         let settings_pages = self.filtered_pages(app).collect_vec();
         let appearance = Appearance::as_ref(app);

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<dyn Element> {
+        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<AboutPageView> = app.add_view(window_id, AboutPageView::new);
+        let about_2: ViewHandle<AboutPageView> = 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<Menu<SettingsAction>> =
+            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"
+        );
+    });
+}