From d6d9e5969ccd369b2028c386717f4143ecfa615c Mon Sep 17 00:00:00 2001 From: Oz Date: Tue, 11 Aug 2026 18:30:04 +0000 Subject: [PATCH 1/4] Fix Settings/Rules panes going stale after cross-window tab drag (APP-5311) When a Settings (or AI-facts "Rules") tab is dragged into a new window, the transfer skips the normal PaneContent::detach/attach hooks (Workspace::prepare_for_transferred_tab_attach suppresses detach on the source window). This left three bugs: A. SettingsPaneManager/AIFactManager kept a stale locator on the source window pointing at a pane group that no longer lived there, so reopening Settings/Rules from that window silently no-op'd forever after the transferred tab was closed. B. A transferred SettingsView's SettingsViewEvent subscription remained bound to the workspace that originally created it, so actions taken from a Settings pane now hosted in window B (e.g. clicking "Rules") executed in the stale window A instead. C. The AIFactManager equivalent of A, compounded by B's routing bug. Fix: - PaneGroup::on_window_transferred now re-keys the SettingsPaneManager/ AIFactManager locator from the source window to the destination window, and re-homes the SettingsView/AIFactView event subscription from the source workspace to the destination workspace. - Workspace::open_settings_pane and open_ai_fact_collection_pane are now defensive: if the registered locator does not resolve to a live pane in the current window, they clear it and open a fresh tab/pane instead of silently doing nothing. Added regression tests in app/src/workspace/view_tests.rs covering all three symptoms via the same cross-window transfer primitives production code uses (transfer_view_tree_to_window + insert_transferred_tab_at_index + remove_tab_without_undo). Co-Authored-By: Warp Agent --- app/src/pane_group/mod.rs | 90 ++++++++- app/src/pane_group/pane/settings_pane.rs | 2 +- app/src/workspace/view.rs | 90 ++++++--- app/src/workspace/view_tests.rs | 235 +++++++++++++++++++++++ 4 files changed, 390 insertions(+), 27 deletions(-) diff --git a/app/src/pane_group/mod.rs b/app/src/pane_group/mod.rs index 49757139cba..daa0a8f7102 100644 --- a/app/src/pane_group/mod.rs +++ b/app/src/pane_group/mod.rs @@ -62,6 +62,7 @@ use crate::ai::blocklist::{BlocklistAIHistoryModel, InputConfig, SerializedBlock use crate::ai::document::ai_document_model::{AIDocumentId, AIDocumentModel, AIDocumentVersion}; use crate::ai::execution_profiles::ExecutionProfileId; use crate::ai::execution_profiles::profiles::AIExecutionProfilesModel; +use crate::ai::facts::AIFactManager; use crate::ai::llms::LLMId; use crate::ai::restored_conversations::RestoredAgentConversations; use crate::ai_assistant::AskAIType; @@ -116,6 +117,7 @@ use crate::session_management::SessionNavigationData; use crate::settings::{AISettings, DefaultSessionMode, PaneSettings}; use crate::settings_view::SettingsSection; use crate::settings_view::mcp_servers_page::MCPServersSettingsPage; +use crate::settings_view::pane_manager::SettingsPaneManager; use crate::shell_indicator::ShellIndicatorType; use crate::terminal::available_shells::{AvailableShell, AvailableShells}; #[cfg(not(target_family = "wasm"))] @@ -8217,9 +8219,91 @@ impl View for PaneGroup { fn on_window_transferred( &mut self, - _old_window_id: WindowId, - _new_window_id: WindowId, - _ctx: &mut ViewContext, + old_window_id: WindowId, + new_window_id: WindowId, + ctx: &mut ViewContext, ) { + // `SettingsPaneManager`/`AIFactManager` track at most one live pane of + // each kind per window, keyed by `WindowId`. A tab-drag transfer moves + // the pane's view tree to `new_window_id` without going through the + // normal `PaneContent::detach`/`attach` hooks (see + // `Workspace::prepare_for_transferred_tab_attach`), so without this the + // source window is left with a locator pointing at a pane that no + // longer lives there, and `open_settings_pane`/ + // `open_ai_fact_collection_pane` silently no-op forever afterwards. + // Re-key the registration here instead. + let pane_group_id = ctx.view_id(); + + let settings_pane_ids: Vec = self + .panes_of::() + .map(|pane| pane.id()) + .collect(); + for pane_id in settings_pane_ids { + if let Some(pane) = self.downcast_pane_by_id::(pane_id) { + let settings_view = pane.settings_view(ctx); + SettingsPaneManager::handle(ctx).update(ctx, |manager, ctx| { + manager.deregister_pane(&old_window_id, pane_group_id, pane_id, ctx); + manager.register_pane(pane, pane_group_id, new_window_id, ctx); + }); + + // `SettingsView`'s `SettingsViewEvent` subscription is + // registered once, in `Workspace::build_settings_views`, + // against whichever workspace created it. That subscription + // does not follow the view when it transfers to another + // window, so actions taken from a transferred Settings pane + // (e.g. clicking "Rules") would otherwise execute in the + // stale, original window. Re-home the subscription to the + // workspace that now hosts the pane. See APP-5311. + if let Some(old_workspace) = + workspace::WorkspaceRegistry::as_ref(ctx).get(old_window_id, ctx) + { + old_workspace.update(ctx, |_, ctx| { + ctx.unsubscribe_to_view(&settings_view); + }); + } + if let Some(new_workspace) = + workspace::WorkspaceRegistry::as_ref(ctx).get(new_window_id, ctx) + { + new_workspace.update(ctx, |_, ctx| { + ctx.subscribe_to_view(&settings_view, move |me, _, event, ctx| { + me.handle_settings_pane_event(event, ctx); + }); + }); + } + } + } + + let ai_fact_pane_ids: Vec = self + .panes_of::() + .map(|pane| pane.id()) + .collect(); + for pane_id in ai_fact_pane_ids { + if let Some(pane) = self.downcast_pane_by_id::(pane_id) { + let ai_fact_view = pane.ai_fact_view(ctx); + AIFactManager::handle(ctx).update(ctx, |manager, ctx| { + manager.deregister_pane(&old_window_id, ctx); + manager.register_pane(pane, pane_group_id, new_window_id, ctx); + }); + + // Same re-homing as above, for the AI fact (Rules) pane's + // `AIFactViewEvent` subscription. + if let Some(old_workspace) = + workspace::WorkspaceRegistry::as_ref(ctx).get(old_window_id, ctx) + { + old_workspace.update(ctx, |_, ctx| { + ctx.unsubscribe_to_view(&ai_fact_view); + }); + } + if let Some(new_workspace) = + workspace::WorkspaceRegistry::as_ref(ctx).get(new_window_id, ctx) + { + new_workspace.update(ctx, |_, ctx| { + ctx.subscribe_to_view(&ai_fact_view, move |me, _, event, ctx| { + me.handle_ai_fact_view_event(event, ctx); + }); + }); + } + } + } } } diff --git a/app/src/pane_group/pane/settings_pane.rs b/app/src/pane_group/pane/settings_pane.rs index d391f97384c..cad1e34dfe4 100644 --- a/app/src/pane_group/pane/settings_pane.rs +++ b/app/src/pane_group/pane/settings_pane.rs @@ -45,7 +45,7 @@ impl SettingsPane { Self::from_view(view, ctx) } - fn settings_view(&self, ctx: &AppContext) -> ViewHandle { + pub fn settings_view(&self, ctx: &AppContext) -> ViewHandle { self.view.as_ref(ctx).child(ctx) } } diff --git a/app/src/workspace/view.rs b/app/src/workspace/view.rs index f2efdd50b17..9986cbd4f3f 100644 --- a/app/src/workspace/view.rs +++ b/app/src/workspace/view.rs @@ -6276,7 +6276,11 @@ impl Workspace { ctx.focus(&self.header_toolbar_editor_modal); } - fn handle_ai_fact_view_event(&mut self, event: &AIFactViewEvent, ctx: &mut ViewContext) { + pub(crate) fn handle_ai_fact_view_event( + &mut self, + event: &AIFactViewEvent, + ctx: &mut ViewContext, + ) { match event { AIFactViewEvent::OpenSettings => { self.show_settings_with_section(Some(SettingsSection::WarpAgent), ctx); @@ -8596,22 +8600,42 @@ impl Workspace { // Ensure there is only one settings pane per window let settings_pane_manager = SettingsPaneManager::handle(ctx); if let Some(locator) = settings_pane_manager.as_ref(ctx).find_pane(ctx.window_id()) { - // Update the page and/or search query if specified. The search query - // must be applied even when no page is given (e.g. `warp://settings?q=`) - // so an already-open settings tab reflects the new query. - if page.is_some() || search_query.is_some() { - self.settings_pane.update(ctx, |settings_pane, ctx| { - if let Some(page) = page { - settings_pane.set_and_refresh_current_page(page, ctx); - } - if let Some(search_query) = search_query { - settings_pane.set_search_query(search_query, ctx); - } - }); + let pane_is_live = self.tabs.iter().any(|tab| { + tab.pane_group.id() == locator.pane_group_id + && tab + .pane_group + .as_ref(ctx) + .pane_by_id(locator.pane_id) + .is_some() + }); + if pane_is_live { + // Update the page and/or search query if specified. The search query + // must be applied even when no page is given (e.g. `warp://settings?q=`) + // so an already-open settings tab reflects the new query. + if page.is_some() || search_query.is_some() { + self.settings_pane.update(ctx, |settings_pane, ctx| { + if let Some(page) = page { + settings_pane.set_and_refresh_current_page(page, ctx); + } + if let Some(search_query) = search_query { + settings_pane.set_search_query(search_query, ctx); + } + }); + } + // Navigate to and focus existing pane + self.focus_pane(locator, ctx); + return; } - // Navigate to and focus existing pane - self.focus_pane(locator, ctx); - return; + + // The registered locator no longer resolves to a live tab in this + // window (e.g. the settings tab was dragged into another window). + // Clear it so we fall through to opening a fresh settings tab + // below, instead of silently no-op'ing forever. See APP-5311. + let window_id = ctx.window_id(); + log::warn!("Clearing stale settings pane locator for window {window_id:?}"); + settings_pane_manager.update(ctx, |manager, ctx| { + manager.deregister_pane(&window_id, locator.pane_group_id, locator.pane_id, ctx); + }); } let ps1_grid_info = self.active_session_ps1_grid_info(ctx); @@ -8958,13 +8982,33 @@ impl Workspace { // Navigate to and focus existing pane if let Some(locator) = manager.as_ref(ctx).find_pane(ctx.window_id()) { - if let Some(page) = page { - self.ai_fact_view.update(ctx, |view, ctx| { - view.update_page(page, ctx); - }); + let pane_is_live = self.tabs.iter().any(|tab| { + tab.pane_group.id() == locator.pane_group_id + && tab + .pane_group + .as_ref(ctx) + .pane_by_id(locator.pane_id) + .is_some() + }); + if pane_is_live { + if let Some(page) = page { + self.ai_fact_view.update(ctx, |view, ctx| { + view.update_page(page, ctx); + }); + } + self.focus_pane(locator, ctx); + return; } - self.focus_pane(locator, ctx); - return; + + // The registered locator no longer resolves to a live tab in this + // window (e.g. the pane was dragged into another window as part + // of a tab transfer). Clear it so we fall through to opening a + // fresh pane below, instead of silently no-op'ing forever. + let window_id = ctx.window_id(); + log::warn!("Clearing stale AI fact pane locator for window {window_id:?}"); + manager.update(ctx, |manager, ctx| { + manager.deregister_pane(&window_id, ctx); + }); } let pane = AIFactPane::from_view(self.ai_fact_view.clone(), ctx); @@ -15106,7 +15150,7 @@ impl Workspace { }) } - fn handle_settings_pane_event( + pub(crate) fn handle_settings_pane_event( &mut self, event: &SettingsViewEvent, ctx: &mut ViewContext, diff --git a/app/src/workspace/view_tests.rs b/app/src/workspace/view_tests.rs index eb20ba4ad77..b567790be03 100644 --- a/app/src/workspace/view_tests.rs +++ b/app/src/workspace/view_tests.rs @@ -4845,3 +4845,238 @@ fn test_tools_panel_warp_drive_toggle_updates_available_views() { }); }); } + +/// Transfers the tab at `tab_index` from `source` to `target`, mirroring the +/// production cross-window tab-drag handoff +/// (`CrossWindowTabDrag::execute_handoff_single_tab_to_other` + +/// `Workspace::handle_drop_result(DropResult::RemoveSourceTab)`): the pane +/// group's view tree is moved to the target window, the target inserts a new +/// tab for it, and the source removes its now-stale tab entry. +fn transfer_tab_to_new_window( + app: &mut App, + source: &ViewHandle, + source_window: WindowId, + target: &ViewHandle, + target_window: WindowId, + tab_index: usize, +) -> ViewHandle { + let transferred_tab = source + .read(app, |ws, ctx| ws.get_tab_transfer_info(tab_index, ctx)) + .expect("tab should be transferable (workspace must have more than one tab)"); + let pane_group = transferred_tab.pane_group.clone(); + let pane_group_id = pane_group.id(); + + source.update(app, |ws, ctx| { + ws.prepare_for_transferred_tab_attach(&transferred_tab.pane_group, ctx); + }); + app.update(|ctx| { + ctx.transfer_view_tree_to_window(pane_group_id, source_window, target_window); + }); + let insertion_index = target.read(app, |ws, _| ws.tab_count()); + target.update(app, |ws, ctx| { + ws.insert_transferred_tab_at_index(transferred_tab, insertion_index, ctx); + }); + source.update(app, |ws, ctx| { + ws.remove_tab_without_undo(tab_index, ctx); + }); + + pane_group +} + +/// Regression for APP-5311, symptom A: after a Settings tab is dragged into +/// another window and closed there, clicking Settings in the original window +/// silently did nothing because `SettingsPaneManager` kept a locator pointing +/// at a pane group that no longer lived in that window. +#[test] +fn test_settings_pane_reopens_after_cross_window_transfer_and_close() { + App::test((), |mut app| async move { + initialize_app(&mut app); + + let workspace_a = mock_workspace(&mut app); + let window_a = workspace_a.update(&mut app, |_, ctx| ctx.window_id()); + let workspace_b = mock_workspace(&mut app); + let window_b = workspace_b.update(&mut app, |_, ctx| ctx.window_id()); + + workspace_a.update(&mut app, |ws, ctx| { + ws.open_settings_pane(None, None, ctx); + }); + let settings_tab_index = workspace_a.read(&app, |ws, _| ws.tab_count() - 1); + + let locator_before_transfer = app + .read(|ctx| SettingsPaneManager::as_ref(ctx).find_pane(window_a)) + .expect("settings pane should be registered for window A"); + + transfer_tab_to_new_window( + &mut app, + &workspace_a, + window_a, + &workspace_b, + window_b, + settings_tab_index, + ); + + // The source window's locator must be cleared and the destination + // window must now own it; otherwise `open_settings_pane` in window A + // resolves a stale locator and silently no-ops. + app.read(|ctx| { + assert_eq!(SettingsPaneManager::as_ref(ctx).find_pane(window_a), None); + assert_eq!( + SettingsPaneManager::as_ref(ctx).find_pane(window_b), + Some(locator_before_transfer) + ); + }); + + // Close the transferred Settings pane in window B (mirrors the + // reported repro's "close the new Settings window" step). + let b_settings_tab_index = workspace_b.read(&app, |ws, _| ws.tab_count() - 1); + workspace_b.update(&mut app, |ws, ctx| { + ws.remove_tab(b_settings_tab_index, false, true, ctx); + }); + app.read(|ctx| { + assert_eq!(SettingsPaneManager::as_ref(ctx).find_pane(window_b), None); + }); + + // Reopening Settings in window A must create a fresh tab rather than + // silently doing nothing. + let tab_count_before_reopen = workspace_a.read(&app, |ws, _| ws.tab_count()); + workspace_a.update(&mut app, |ws, ctx| { + ws.open_settings_pane(None, None, ctx); + }); + let tab_count_after_reopen = workspace_a.read(&app, |ws, _| ws.tab_count()); + assert_eq!(tab_count_after_reopen, tab_count_before_reopen + 1); + }); +} + +/// Regression for APP-5311, symptom B: a `SettingsView`'s event subscription +/// is registered once, against whichever workspace created it. Without +/// re-homing it on transfer, an action taken from a Settings pane living in +/// window B (e.g. clicking "Rules") would execute in the stale window A. +#[test] +fn test_settings_pane_actions_execute_in_hosting_window_after_cross_window_transfer() { + App::test((), |mut app| async move { + initialize_app(&mut app); + + let workspace_a = mock_workspace(&mut app); + let window_a = workspace_a.update(&mut app, |_, ctx| ctx.window_id()); + let workspace_b = mock_workspace(&mut app); + let window_b = workspace_b.update(&mut app, |_, ctx| ctx.window_id()); + + workspace_a.update(&mut app, |ws, ctx| { + ws.open_settings_pane(None, None, ctx); + }); + let settings_tab_index = workspace_a.read(&app, |ws, _| ws.tab_count() - 1); + let locator = app + .read(|ctx| SettingsPaneManager::as_ref(ctx).find_pane(window_a)) + .expect("settings pane should be registered for window A"); + + let pane_group = transfer_tab_to_new_window( + &mut app, + &workspace_a, + window_a, + &workspace_b, + window_b, + settings_tab_index, + ); + + // Grab the exact `SettingsView` embedded in the transferred pane, as + // opposed to window B's own native settings view. + let settings_view = pane_group.read(&app, |pane_group, ctx| { + pane_group + .downcast_pane_by_id::(locator.pane_id) + .expect("transferred pane should still be a SettingsPane") + .settings_view(ctx) + }); + + let window_a_pane_count_before = workspace_a.read(&app, |ws, ctx| { + ws.active_tab_pane_group().as_ref(ctx).pane_count() + }); + let pane_count_before = pane_group.read(&app, |pg, _| pg.pane_count()); + + // Simulate clicking "Rules" inside the (now window-B-hosted) Settings + // pane. Before the fix, this event's subscription still pointed at + // workspace A (the window the pane was created in), so the AI-fact + // pane would open in the wrong window. + settings_view.update(&mut app, |_, ctx| { + ctx.emit(SettingsViewEvent::OpenAIFactCollection); + }); + + assert_eq!( + workspace_a.read(&app, |ws, ctx| ws + .active_tab_pane_group() + .as_ref(ctx) + .pane_count()), + window_a_pane_count_before, + "window A should be untouched by an action from the transferred pane" + ); + assert_eq!( + pane_group.read(&app, |pg, _| pg.pane_count()), + pane_count_before + 1, + "the AI fact pane should open as a split alongside Settings in window B" + ); + }); +} + +/// Regression for APP-5311, symptom C: the `AIFactManager` equivalent of +/// symptom A. Moving a tab containing both a Settings pane and a split-off +/// Rules (AI fact) pane to a new window, then closing just the Rules pane, +/// must not leave the source window's locator stale. +#[test] +fn test_ai_fact_pane_reopens_after_cross_window_transfer_and_close() { + App::test((), |mut app| async move { + initialize_app(&mut app); + + let workspace_a = mock_workspace(&mut app); + let window_a = workspace_a.update(&mut app, |_, ctx| ctx.window_id()); + let workspace_b = mock_workspace(&mut app); + let window_b = workspace_b.update(&mut app, |_, ctx| ctx.window_id()); + + // Open Settings, then open Rules as a split pane within the same tab, + // mirroring the reported repro. + workspace_a.update(&mut app, |ws, ctx| { + ws.open_settings_pane(None, None, ctx); + }); + workspace_a.update(&mut app, |ws, ctx| { + ws.open_ai_fact_collection_pane(Some(Direction::Right), None, ctx); + }); + let settings_tab_index = workspace_a.read(&app, |ws, _| ws.tab_count() - 1); + + let locator_before_transfer = app + .read(|ctx| AIFactManager::as_ref(ctx).find_pane(window_a)) + .expect("AI fact pane should be registered for window A"); + + let pane_group = transfer_tab_to_new_window( + &mut app, + &workspace_a, + window_a, + &workspace_b, + window_b, + settings_tab_index, + ); + + app.read(|ctx| { + assert_eq!(AIFactManager::as_ref(ctx).find_pane(window_a), None); + assert_eq!( + AIFactManager::as_ref(ctx).find_pane(window_b), + Some(locator_before_transfer) + ); + }); + + // Close just the Rules pane (not the whole tab) in window B. + pane_group.update(&mut app, |pg, ctx| { + pg.close_pane(locator_before_transfer.pane_id, ctx); + }); + app.read(|ctx| { + assert_eq!(AIFactManager::as_ref(ctx).find_pane(window_b), None); + }); + + // Reopening Rules from window B (e.g. via the Settings "Rules" + // button) must create a fresh pane rather than silently doing + // nothing. + let pane_count_before_reopen = pane_group.read(&app, |pg, _| pg.pane_count()); + workspace_b.update(&mut app, |ws, ctx| { + ws.open_ai_fact_collection_pane(Some(Direction::Right), None, ctx); + }); + let pane_count_after_reopen = pane_group.read(&app, |pg, _| pg.pane_count()); + assert_eq!(pane_count_after_reopen, pane_count_before_reopen + 1); + }); +} From 6346a171a9f914e07c24958ee78e49aa8f721339 Mon Sep 17 00:00:00 2001 From: Oz Date: Tue, 11 Aug 2026 19:59:02 +0000 Subject: [PATCH 2/4] Address review: fix reentrancy panic, wrong-view targeting, and collision handling - PaneGroup::on_window_transferred no longer runs the Settings/AI-fact subscription rehoming synchronously; it's dispatched as a self-targeted deferred PaneGroupAction so a real drag (which runs while the source Workspace is already mid-update) doesn't panic with "Circular view update". - open_settings_pane/open_ai_fact_collection_pane now resolve the concrete SettingsPane/AIFactPane's own view via the locator instead of updating this window's native (possibly different) settings_pane/ai_fact_view, so page/search navigation and OpenSettings/OpenAIFactCollection routing target the pane that's actually hosting the request. - Narrowed SettingsPane::settings_view to pub(crate). - Enforced the one-Settings-pane/one-Rules-pane-per-window invariant on transfer: SettingsPaneManager/AIFactManager gained register_transferred_pane, which detects a collision with an existing pane instead of silently overwriting the destination locator. On collision, the transferred pane is discarded (its whole tab if it was the tab's only content, otherwise just the pane) and the pre-existing pane is kept and focused, via a second self-targeted deferred WorkspaceAction so the pane group being discarded is never touched while still mid-update. - Added regression tests: real single-tab-drag handoff path (proves no panic), page-navigation targeting the transferred pane, and collision reconciliation for both Settings and Rules panes. Co-Authored-By: Warp Agent --- app/src/ai/facts/manager.rs | 54 +++- app/src/pane_group/mod.rs | 259 +++++++++++++++--- app/src/pane_group/pane/ai_fact_pane.rs | 4 +- app/src/pane_group/pane/settings_pane.rs | 2 +- app/src/settings_view/pane_manager.rs | 33 +++ app/src/workspace/action.rs | 18 ++ app/src/workspace/view.rs | 116 ++++++-- app/src/workspace/view_tests.rs | 331 +++++++++++++++++++++++ 8 files changed, 743 insertions(+), 74 deletions(-) diff --git a/app/src/ai/facts/manager.rs b/app/src/ai/facts/manager.rs index 5a292c09baa..07d0624e8cd 100644 --- a/app/src/ai/facts/manager.rs +++ b/app/src/ai/facts/manager.rs @@ -4,10 +4,10 @@ use warpui::{Entity, EntityId, ModelContext, SingletonEntity, ViewHandle, Window use crate::PaneViewLocator; use crate::ai::facts::AIFactView; -use crate::pane_group::{AIFactPane, PaneContent}; +use crate::pane_group::{AIFactPane, PaneContent, PaneId}; -/// Singleton model to manage state of AI fact panes across multiple windows -/// (where only one AI fact pane can exist per window). Specifically: +/// Singleton model to manage state of AI fact panes across multiple windows. +/// Specifically: /// - Maintains AI fact view handles to preserve state when panes are hidden /// - Tracks currently open AI fact panes and their location #[derive(Default)] @@ -68,9 +68,53 @@ impl AIFactManager { } } - pub fn deregister_pane(&mut self, window_id: &WindowId, _ctx: &mut ModelContext) { + /// Registers `pane` as transferred into `window_id`, preserving the + /// invariant that at most one AI fact pane is tracked per window. If + /// `window_id` already has a *different* live pane registered, the + /// existing registration is left untouched and its locator is returned + /// so the caller can reconcile the collision (the transferred pane must + /// be discarded and the existing one kept). `None` means there was no + /// collision -- the slot was empty, or already pointed at this exact + /// pane -- and the transferred pane is now the registered one. + pub fn register_transferred_pane( + &mut self, + pane: &AIFactPane, + pane_group_id: EntityId, + window_id: WindowId, + _ctx: &mut ModelContext, + ) -> Option { + let incoming = PaneViewLocator { + pane_group_id, + pane_id: pane.id(), + }; + let Some(data) = self.panes.get_mut(&window_id) else { + log::warn!("AI fact view should already exist for AI fact pane"); + return None; + }; + match data.locator { + Some(existing) if existing != incoming => Some(existing), + _ => { + data.locator = Some(incoming); + None + } + } + } + + pub fn deregister_pane( + &mut self, + window_id: &WindowId, + pane_group_id: EntityId, + pane_id: PaneId, + _ctx: &mut ModelContext, + ) { if let Some(data) = self.panes.get_mut(window_id) { - data.locator = None; + let locator = PaneViewLocator { + pane_group_id, + pane_id, + }; + if data.locator == Some(locator) { + data.locator = None; + } } } } diff --git a/app/src/pane_group/mod.rs b/app/src/pane_group/mod.rs index daa0a8f7102..b36bd8afb1e 100644 --- a/app/src/pane_group/mod.rs +++ b/app/src/pane_group/mod.rs @@ -62,7 +62,7 @@ use crate::ai::blocklist::{BlocklistAIHistoryModel, InputConfig, SerializedBlock use crate::ai::document::ai_document_model::{AIDocumentId, AIDocumentModel, AIDocumentVersion}; use crate::ai::execution_profiles::ExecutionProfileId; use crate::ai::execution_profiles::profiles::AIExecutionProfilesModel; -use crate::ai::facts::AIFactManager; +use crate::ai::facts::{AIFactManager, AIFactView}; use crate::ai::llms::LLMId; use crate::ai::restored_conversations::RestoredAgentConversations; use crate::ai_assistant::AskAIType; @@ -115,9 +115,9 @@ use crate::server::telemetry::{ }; use crate::session_management::SessionNavigationData; use crate::settings::{AISettings, DefaultSessionMode, PaneSettings}; -use crate::settings_view::SettingsSection; use crate::settings_view::mcp_servers_page::MCPServersSettingsPage; use crate::settings_view::pane_manager::SettingsPaneManager; +use crate::settings_view::{SettingsSection, SettingsView}; use crate::shell_indicator::ShellIndicatorType; use crate::terminal::available_shells::{AvailableShell, AvailableShells}; #[cfg(not(target_family = "wasm"))] @@ -320,6 +320,54 @@ pub enum PaneGroupAction { ToggleMaximizePane, HandleFocusChange, FocusTerminalView(EntityId), + /// Re-homes a transferred Settings/AI-fact pane's workspace-level event + /// subscription from the source workspace to the destination workspace. + /// Always dispatched via `ViewContext::dispatch_typed_action_deferred` + /// from `on_window_transferred` as a *self*-targeted action (i.e. this + /// pane group is both the dispatcher and the handler), never run + /// synchronously -- see that method's doc comment for why. Self- + /// targeting matters, not just deferral: unlike a `WorkspaceAction` + /// dispatched from here, it doesn't depend on this pane group's + /// render-time parent link being registered in the destination window + /// (which isn't established until the next render pass), so it + /// reliably reaches this handler regardless of render timing. + RehomePaneEventSubscription(PaneEventSubscriptionRehome), +} + +/// Payload for [`PaneGroupAction::RehomePaneEventSubscription`]. +#[derive(Debug, Clone)] +pub enum PaneEventSubscriptionRehome { + Settings { + old_window_id: WindowId, + new_window_id: WindowId, + settings_view: ViewHandle, + /// Set when the destination window already hosted a different, live + /// Settings pane at transfer time. Warp enforces at most one + /// Settings pane per window (an intentional invariant, not a bug), + /// so the transferred pane must be discarded and the pre-existing + /// one focused instead of leaving two live panes. See APP-5311. + collision: Option, + }, + AIFact { + old_window_id: WindowId, + new_window_id: WindowId, + ai_fact_view: ViewHandle, + /// Same as `Settings::collision`, for the AI fact (Rules) pane. + collision: Option, + }, +} + +/// Identifies which of two colliding Settings/AI-fact panes survives after a +/// transfer, so the one-pane-per-window invariant holds. See +/// [`PaneEventSubscriptionRehome`]. +#[derive(Debug, Clone, Copy)] +pub struct PaneCollisionReconciliation { + /// The pane that already existed in the destination window before the + /// transfer; the survivor. + pub keep: PaneViewLocator, + /// The just-transferred pane, which lost the collision and must be + /// discarded. + pub discard: PaneViewLocator, } #[derive(PartialEq)] enum PaneRemovalReason { @@ -8054,6 +8102,9 @@ impl TypedActionView for PaneGroup { } => self.move_pane(*id, *target_pane_id, *direction, ctx), HandleFocusChange => self.handle_focus_change(ctx), FocusTerminalView(terminal_view_id) => self.focus_terminal_view(*terminal_view_id, ctx), + RehomePaneEventSubscription(rehome) => { + Self::rehome_pane_event_subscription(rehome, ctx) + } } } } @@ -8223,15 +8274,22 @@ impl View for PaneGroup { new_window_id: WindowId, ctx: &mut ViewContext, ) { - // `SettingsPaneManager`/`AIFactManager` track at most one live pane of - // each kind per window, keyed by `WindowId`. A tab-drag transfer moves - // the pane's view tree to `new_window_id` without going through the - // normal `PaneContent::detach`/`attach` hooks (see - // `Workspace::prepare_for_transferred_tab_attach`), so without this the - // source window is left with a locator pointing at a pane that no - // longer lives there, and `open_settings_pane`/ + // `SettingsPaneManager`/`AIFactManager` track at most one live pane + // of each kind per window, keyed by `WindowId` -- Warp enforces one + // Settings pane and one Rules pane per window as an intentional + // invariant. A tab-drag transfer moves the pane's view tree to + // `new_window_id` without going through the normal + // `PaneContent::detach`/`attach` hooks (see + // `Workspace::prepare_for_transferred_tab_attach`), so without this + // the source window is left with a locator pointing at a pane that + // no longer lives there, and `open_settings_pane`/ // `open_ai_fact_collection_pane` silently no-op forever afterwards. - // Re-key the registration here instead. + // Re-key the registration here instead. If the destination window + // already has a live pane of the same kind, `register_transferred_pane` + // leaves that existing registration untouched and reports the + // collision so it can be reconciled below (the transferred pane is + // discarded and the pre-existing one kept), instead of silently + // overwriting the destination locator while both panes stay live. let pane_group_id = ctx.view_id(); let settings_pane_ids: Vec = self @@ -8241,35 +8299,57 @@ impl View for PaneGroup { for pane_id in settings_pane_ids { if let Some(pane) = self.downcast_pane_by_id::(pane_id) { let settings_view = pane.settings_view(ctx); - SettingsPaneManager::handle(ctx).update(ctx, |manager, ctx| { + let collision = SettingsPaneManager::handle(ctx).update(ctx, |manager, ctx| { manager.deregister_pane(&old_window_id, pane_group_id, pane_id, ctx); - manager.register_pane(pane, pane_group_id, new_window_id, ctx); + manager + .register_transferred_pane(pane, pane_group_id, new_window_id, ctx) + .map(|keep| PaneCollisionReconciliation { + keep, + discard: PaneViewLocator { + pane_group_id, + pane_id, + }, + }) }); // `SettingsView`'s `SettingsViewEvent` subscription is // registered once, in `Workspace::build_settings_views`, - // against whichever workspace created it. That subscription - // does not follow the view when it transfers to another - // window, so actions taken from a transferred Settings pane - // (e.g. clicking "Rules") would otherwise execute in the - // stale, original window. Re-home the subscription to the - // workspace that now hosts the pane. See APP-5311. - if let Some(old_workspace) = - workspace::WorkspaceRegistry::as_ref(ctx).get(old_window_id, ctx) - { - old_workspace.update(ctx, |_, ctx| { - ctx.unsubscribe_to_view(&settings_view); - }); - } - if let Some(new_workspace) = - workspace::WorkspaceRegistry::as_ref(ctx).get(new_window_id, ctx) - { - new_workspace.update(ctx, |_, ctx| { - ctx.subscribe_to_view(&settings_view, move |me, _, event, ctx| { - me.handle_settings_pane_event(event, ctx); - }); - }); - } + // against whichever workspace created it, and does not follow + // the view when it transfers to another window -- so actions + // taken from a transferred Settings pane (e.g. clicking + // "Rules") would otherwise execute in the stale, original + // window. Re-home the subscription to the workspace that now + // hosts the pane (or, on collision, discard the transferred + // pane instead -- see `PaneCollisionReconciliation`). + // + // This MUST be deferred rather than run synchronously here: + // a real drag-and-drop transfer runs while the source (and + // sometimes destination) `Workspace` view is still mid-update + // further up the call stack (e.g. + // `Workspace::handle_action(DropTab)` -> + // `perform_handoff` -> `CrossWindowTabDrag:: + // execute_handoff_single_tab_to_other` -> + // `AppContext::transfer_view_tree_to_window` -> here), which + // means that workspace's view has already been removed from + // its window's view map. Calling `ViewHandle::update` on it + // again re-entrantly would panic with "Circular view + // update". Self-targeted (dispatched as a `PaneGroupAction` + // back to this same pane group, not a `WorkspaceAction`) + // because ancestor-chain-based dispatch depends on this pane + // group's render-time parent link in the destination + // window, which isn't registered until the next render pass + // -- unreliable right after a transfer. `dispatch_typed_ + // action_deferred` queues the action to run once the + // current update finishes and this pane group's view has + // been reinserted. See APP-5311. + ctx.dispatch_typed_action_deferred(PaneGroupAction::RehomePaneEventSubscription( + PaneEventSubscriptionRehome::Settings { + old_window_id, + new_window_id, + settings_view, + collision, + }, + )); } } @@ -8280,29 +8360,120 @@ impl View for PaneGroup { for pane_id in ai_fact_pane_ids { if let Some(pane) = self.downcast_pane_by_id::(pane_id) { let ai_fact_view = pane.ai_fact_view(ctx); - AIFactManager::handle(ctx).update(ctx, |manager, ctx| { - manager.deregister_pane(&old_window_id, ctx); - manager.register_pane(pane, pane_group_id, new_window_id, ctx); + let collision = AIFactManager::handle(ctx).update(ctx, |manager, ctx| { + manager.deregister_pane(&old_window_id, pane_group_id, pane_id, ctx); + manager + .register_transferred_pane(pane, pane_group_id, new_window_id, ctx) + .map(|keep| PaneCollisionReconciliation { + keep, + discard: PaneViewLocator { + pane_group_id, + pane_id, + }, + }) }); - // Same re-homing as above, for the AI fact (Rules) pane's + // Same re-homing (and collision handling) as above, deferred + // for the same reason, for the AI fact (Rules) pane's // `AIFactViewEvent` subscription. + ctx.dispatch_typed_action_deferred(PaneGroupAction::RehomePaneEventSubscription( + PaneEventSubscriptionRehome::AIFact { + old_window_id, + new_window_id, + ai_fact_view, + collision, + }, + )); + } + } + } +} + +impl PaneGroup { + /// Performs the actual work for [`PaneGroupAction::RehomePaneEventSubscription`]. + /// Invoked as `self`'s own `handle_action`, so `self` (this pane group) + /// is mid-update for the duration of this call -- touching it again here + /// (e.g. to discard a colliding duplicate) would panic with "Circular + /// view update". The non-collision branches only touch the old/new + /// *workspace* views, which are safe to update directly. The collision + /// branch instead re-homes to a second, self-targeted deferred dispatch + /// on the *destination workspace* (`WorkspaceAction:: + /// DiscardDuplicateTransferredPane`), which reliably reaches `Workspace:: + /// handle_action` (unlike an ancestor-chain dispatch from here -- see + /// `on_window_transferred`) and, by the time it runs, finds this pane + /// group's view safely reinserted. + fn rehome_pane_event_subscription( + rehome: &PaneEventSubscriptionRehome, + ctx: &mut ViewContext, + ) { + match rehome { + PaneEventSubscriptionRehome::Settings { + old_window_id, + new_window_id, + settings_view, + collision, + } => { if let Some(old_workspace) = - workspace::WorkspaceRegistry::as_ref(ctx).get(old_window_id, ctx) + workspace::WorkspaceRegistry::as_ref(ctx).get(*old_window_id, ctx) { old_workspace.update(ctx, |_, ctx| { - ctx.unsubscribe_to_view(&ai_fact_view); + ctx.unsubscribe_to_view(settings_view); }); } if let Some(new_workspace) = - workspace::WorkspaceRegistry::as_ref(ctx).get(new_window_id, ctx) + workspace::WorkspaceRegistry::as_ref(ctx).get(*new_window_id, ctx) { - new_workspace.update(ctx, |_, ctx| { - ctx.subscribe_to_view(&ai_fact_view, move |me, _, event, ctx| { - me.handle_ai_fact_view_event(event, ctx); + if let Some(collision) = collision { + let keep = collision.keep; + let discard = collision.discard; + new_workspace.update(ctx, |_, ctx| { + ctx.dispatch_typed_action_deferred( + WorkspaceAction::DiscardDuplicateTransferredPane { keep, discard }, + ); }); + } else { + let settings_view = settings_view.clone(); + new_workspace.update(ctx, |_, ctx| { + ctx.subscribe_to_view(&settings_view, move |me, _, event, ctx| { + me.handle_settings_pane_event(event, ctx); + }); + }); + } + } + } + PaneEventSubscriptionRehome::AIFact { + old_window_id, + new_window_id, + ai_fact_view, + collision, + } => { + if let Some(old_workspace) = + workspace::WorkspaceRegistry::as_ref(ctx).get(*old_window_id, ctx) + { + old_workspace.update(ctx, |_, ctx| { + ctx.unsubscribe_to_view(ai_fact_view); }); } + if let Some(new_workspace) = + workspace::WorkspaceRegistry::as_ref(ctx).get(*new_window_id, ctx) + { + if let Some(collision) = collision { + let keep = collision.keep; + let discard = collision.discard; + new_workspace.update(ctx, |_, ctx| { + ctx.dispatch_typed_action_deferred( + WorkspaceAction::DiscardDuplicateTransferredPane { keep, discard }, + ); + }); + } else { + let ai_fact_view = ai_fact_view.clone(); + new_workspace.update(ctx, |_, ctx| { + ctx.subscribe_to_view(&ai_fact_view, move |me, _, event, ctx| { + me.handle_ai_fact_view_event(event, ctx); + }); + }); + } + } } } } diff --git a/app/src/pane_group/pane/ai_fact_pane.rs b/app/src/pane_group/pane/ai_fact_pane.rs index 00000d839de..5f930d36acc 100644 --- a/app/src/pane_group/pane/ai_fact_pane.rs +++ b/app/src/pane_group/pane/ai_fact_pane.rs @@ -84,9 +84,11 @@ impl PaneContent for AIFactPane { ctx.unsubscribe_to_view(&self.view); // Always deregister from AIFactManager - it will be re-registered on attach if restored + let pane_id = self.id(); + let pane_group_id = ctx.view_id(); let window_id = ctx.window_id(); AIFactManager::handle(ctx).update(ctx, |manager, ctx| { - manager.deregister_pane(&window_id, ctx); + manager.deregister_pane(&window_id, pane_group_id, pane_id, ctx); }); } diff --git a/app/src/pane_group/pane/settings_pane.rs b/app/src/pane_group/pane/settings_pane.rs index cad1e34dfe4..439b66e3b8e 100644 --- a/app/src/pane_group/pane/settings_pane.rs +++ b/app/src/pane_group/pane/settings_pane.rs @@ -45,7 +45,7 @@ impl SettingsPane { Self::from_view(view, ctx) } - pub fn settings_view(&self, ctx: &AppContext) -> ViewHandle { + pub(crate) fn settings_view(&self, ctx: &AppContext) -> ViewHandle { self.view.as_ref(ctx).child(ctx) } } diff --git a/app/src/settings_view/pane_manager.rs b/app/src/settings_view/pane_manager.rs index 7520e2f52c6..79cbb49b0f9 100644 --- a/app/src/settings_view/pane_manager.rs +++ b/app/src/settings_view/pane_manager.rs @@ -5,6 +5,7 @@ use warpui::{Entity, EntityId, ModelContext, SingletonEntity, ViewHandle, Window use super::SettingsView; use crate::PaneViewLocator; use crate::pane_group::{PaneContent, PaneId, SettingsPane}; + struct SettingsPaneData { locator: Option, settings_view: ViewHandle, @@ -67,6 +68,38 @@ impl SettingsPaneManager { } } + /// Registers `pane` as transferred into `window_id`, preserving the + /// invariant that at most one Settings pane is tracked per window. If + /// `window_id` already has a *different* live pane registered, the + /// existing registration is left untouched and its locator is returned + /// so the caller can reconcile the collision (the transferred pane must + /// be discarded and the existing one kept). `None` means there was no + /// collision -- the slot was empty, or already pointed at this exact + /// pane -- and the transferred pane is now the registered one. + pub fn register_transferred_pane( + &mut self, + pane: &SettingsPane, + pane_group_id: EntityId, + window_id: WindowId, + _ctx: &mut ModelContext, + ) -> Option { + let incoming = PaneViewLocator { + pane_group_id, + pane_id: pane.id(), + }; + let Some(data) = self.panes.get_mut(&window_id) else { + log::warn!("Settings view should already exist for settings pane"); + return None; + }; + match data.locator { + Some(existing) if existing != incoming => Some(existing), + _ => { + data.locator = Some(incoming); + None + } + } + } + pub fn deregister_pane( &mut self, window_id: &WindowId, diff --git a/app/src/workspace/action.rs b/app/src/workspace/action.rs index 2982ffc10df..6cc05070da6 100644 --- a/app/src/workspace/action.rs +++ b/app/src/workspace/action.rs @@ -126,6 +126,21 @@ pub enum AutoCloudHandoffTrigger { #[derive(Debug, Clone)] pub enum WorkspaceAction { + /// Reconciles a Settings/AI-fact pane transfer that collided with a live + /// pane of the same kind already in this window (Warp enforces at most + /// one of each per window): discards the just-transferred `discard` + /// pane and focuses the pre-existing `keep` pane. Always dispatched, as + /// a *self*-targeted deferred action, from + /// `PaneGroup::rehome_pane_event_subscription`; see that method's doc + /// comment for why. Self-targeting (rather than reaching this from an + /// ancestor-chain dispatch) is what lets the handler safely touch the + /// just-transferred pane group -- it no longer depends on that pane + /// group's render-time parent link in this window, which isn't + /// registered until the next render pass. + DiscardDuplicateTransferredPane { + keep: PaneViewLocator, + discard: PaneViewLocator, + }, ActivateTab(usize), ActivatePrevTab, ActivateNextTab, @@ -1249,6 +1264,9 @@ impl WorkspaceAction { #[cfg(feature = "local_fs")] FileDeleted { .. } => false, // File deletion doesn't change workspace state OpenEnvironmentManagementPane => false, + // Internal bookkeeping dispatched by `PaneGroup::rehome_pane_event_subscription`, + // not a user action; doesn't reflect a change worth persisting. + DiscardDuplicateTransferredPane { .. } => false, #[cfg(target_os = "linux")] DismissWaylandCrashRecoveryBannerAndOpenLink => false, #[cfg(target_family = "wasm")] diff --git a/app/src/workspace/view.rs b/app/src/workspace/view.rs index 9986cbd4f3f..e007bccff38 100644 --- a/app/src/workspace/view.rs +++ b/app/src/workspace/view.rs @@ -300,7 +300,7 @@ use crate::pane_group::{ self, AIFactPane, AnyPaneContent, ChildAgentOrigin, CodeDiffPane, CodePane, CodeReviewPanelArg, CustomRouterEditorPane, Direction as PaneGroupDirection, Direction, EnvironmentManagementPane, ExecutionProfileEditorPane, NetworkLogPane, NewTerminalOptions, PaneGroup, PaneId, PanesLayout, - TabBarHoverIndex, TerminalPaneId, + SettingsPane, TabBarHoverIndex, TerminalPaneId, }; use crate::persistence::ModelEvent; use crate::projects::ProjectManagementModel; @@ -8591,6 +8591,27 @@ impl Workspace { } } + /// Resolves `locator` to its live `SettingsPane`'s inner `SettingsView`, + /// if the pane group and pane it points at still exist in this + /// workspace's tabs. A locator can be registered but no longer resolve + /// to a live pane via paths other than the normal detach hook (e.g. see + /// APP-5311), so callers must always handle `None`. + fn live_settings_view_for_locator( + &self, + locator: PaneViewLocator, + ctx: &AppContext, + ) -> Option> { + self.tabs + .iter() + .find(|tab| tab.pane_group.id() == locator.pane_group_id) + .and_then(|tab| { + tab.pane_group + .as_ref(ctx) + .downcast_pane_by_id::(locator.pane_id) + }) + .map(|pane| pane.settings_view(ctx)) + } + fn open_settings_pane( &mut self, page: Option, @@ -8600,20 +8621,15 @@ impl Workspace { // Ensure there is only one settings pane per window let settings_pane_manager = SettingsPaneManager::handle(ctx); if let Some(locator) = settings_pane_manager.as_ref(ctx).find_pane(ctx.window_id()) { - let pane_is_live = self.tabs.iter().any(|tab| { - tab.pane_group.id() == locator.pane_group_id - && tab - .pane_group - .as_ref(ctx) - .pane_by_id(locator.pane_id) - .is_some() - }); - if pane_is_live { + if let Some(settings_view) = self.live_settings_view_for_locator(locator, ctx) { // Update the page and/or search query if specified. The search query // must be applied even when no page is given (e.g. `warp://settings?q=`) - // so an already-open settings tab reflects the new query. + // so an already-open settings tab reflects the new query. Resolve the + // located pane's own view rather than this window's native + // `self.settings_pane`, which may be a different, non-rendered + // instance if the located pane was dragged in from another window. if page.is_some() || search_query.is_some() { - self.settings_pane.update(ctx, |settings_pane, ctx| { + settings_view.update(ctx, |settings_pane, ctx| { if let Some(page) = page { settings_pane.set_and_refresh_current_page(page, ctx); } @@ -8970,6 +8986,27 @@ impl Workspace { } } + /// Resolves `locator` to its live `AIFactPane`'s inner `AIFactView`, if + /// the pane group and pane it points at still exist in this workspace's + /// tabs. A locator can be registered but no longer resolve to a live + /// pane via paths other than the normal detach hook (e.g. see + /// APP-5311), so callers must always handle `None`. + fn live_ai_fact_view_for_locator( + &self, + locator: PaneViewLocator, + ctx: &AppContext, + ) -> Option> { + self.tabs + .iter() + .find(|tab| tab.pane_group.id() == locator.pane_group_id) + .and_then(|tab| { + tab.pane_group + .as_ref(ctx) + .downcast_pane_by_id::(locator.pane_id) + }) + .map(|pane| pane.ai_fact_view(ctx)) + } + /// Open the AI Fact Collection pane in a split pane (default direction is left). pub fn open_ai_fact_collection_pane( &mut self, @@ -8982,17 +9019,13 @@ impl Workspace { // Navigate to and focus existing pane if let Some(locator) = manager.as_ref(ctx).find_pane(ctx.window_id()) { - let pane_is_live = self.tabs.iter().any(|tab| { - tab.pane_group.id() == locator.pane_group_id - && tab - .pane_group - .as_ref(ctx) - .pane_by_id(locator.pane_id) - .is_some() - }); - if pane_is_live { + if let Some(ai_fact_view) = self.live_ai_fact_view_for_locator(locator, ctx) { + // Resolve the located pane's own view rather than this + // window's native `self.ai_fact_view`, which may be a + // different, non-rendered instance if the located pane was + // dragged in from another window. if let Some(page) = page { - self.ai_fact_view.update(ctx, |view, ctx| { + ai_fact_view.update(ctx, |view, ctx| { view.update_page(page, ctx); }); } @@ -9007,7 +9040,7 @@ impl Workspace { let window_id = ctx.window_id(); log::warn!("Clearing stale AI fact pane locator for window {window_id:?}"); manager.update(ctx, |manager, ctx| { - manager.deregister_pane(&window_id, ctx); + manager.deregister_pane(&window_id, locator.pane_group_id, locator.pane_id, ctx); }); } @@ -12089,6 +12122,40 @@ impl Workspace { true } + /// Reconciles a Settings/AI-fact pane transfer that collided with a pane + /// of the same kind already live in this window: Warp enforces at most + /// one Settings pane and one Rules pane per window, so the two panes + /// cannot coexist. Removes the just-transferred `discard` pane -- closing + /// its whole tab if it was the tab's only pane, or just the pane + /// otherwise -- and focuses the pre-existing `keep` pane. Invoked via + /// `WorkspaceAction::DiscardDuplicateTransferredPane`; see + /// `PaneGroup::rehome_pane_event_subscription` and + /// `PaneCollisionReconciliation` for why that dispatch is self-targeted + /// on this workspace rather than run synchronously. + fn discard_duplicate_transferred_pane( + &mut self, + keep: PaneViewLocator, + discard: PaneViewLocator, + ctx: &mut ViewContext, + ) { + let discard_tab = self + .tabs + .iter() + .enumerate() + .find(|(_, tab)| tab.pane_group.id() == discard.pane_group_id) + .map(|(index, tab)| (index, tab.pane_group.clone())); + if let Some((index, pane_group)) = discard_tab { + if pane_group.as_ref(ctx).pane_count() <= 1 { + self.remove_tab(index, false, true, ctx); + } else { + pane_group.update(ctx, |pane_group, ctx| { + pane_group.close_pane(discard.pane_id, ctx); + }); + } + } + self.focus_pane(keep, ctx); + } + fn remove_tab( &mut self, index: usize, @@ -23902,6 +23969,9 @@ impl TypedActionView for Workspace { } match action { + DiscardDuplicateTransferredPane { keep, discard } => { + self.discard_duplicate_transferred_pane(*keep, *discard, ctx); + } ActivateTab(index) => self.activate_tab(*index, ctx), ActivateTabByNumber(num) => self.activate_tab(num.saturating_sub(1), ctx), ActivatePrevTab => self.activate_prev_tab(ctx), diff --git a/app/src/workspace/view_tests.rs b/app/src/workspace/view_tests.rs index b567790be03..e72ed601473 100644 --- a/app/src/workspace/view_tests.rs +++ b/app/src/workspace/view_tests.rs @@ -5080,3 +5080,334 @@ fn test_ai_fact_pane_reopens_after_cross_window_transfer_and_close() { assert_eq!(pane_count_after_reopen, pane_count_before_reopen + 1); }); } + +/// Regression for APP-5311 review finding #1 (critical): a real drag runs the +/// view-tree transfer while the *source* `Workspace` is already mid-update +/// (inside `Workspace::perform_handoff`, itself invoked from `handle_action`). +/// `PaneGroup::on_window_transferred` must not call `old_workspace.update(...)` +/// synchronously in that situation, because the source workspace's view has +/// been removed from its window's view map for the duration of that update; +/// re-entering it panics with "Circular view update". This drives the real +/// `perform_handoff` entry point -- unlike `transfer_tab_to_new_window`, which +/// runs the transfer from a fresh top-level `app.update` and so cannot +/// reproduce this call stack. +#[test] +fn test_settings_pane_transfer_via_real_handoff_path_does_not_panic() { + App::test((), |mut app| async move { + initialize_app(&mut app); + + let workspace_a = mock_workspace(&mut app); + let window_a = workspace_a.update(&mut app, |_, ctx| ctx.window_id()); + let workspace_b = mock_workspace(&mut app); + let window_b = workspace_b.update(&mut app, |_, ctx| ctx.window_id()); + + workspace_a.update(&mut app, |ws, ctx| { + ws.open_settings_pane(None, None, ctx); + }); + // Make Settings window A's only tab, matching `DragSource::SingleTabWindow` + // semantics (source_tab_index() == 0) for the single-tab handoff branch below. + workspace_a.update(&mut app, |ws, ctx| { + ws.remove_tab_without_undo(0, ctx); + }); + assert_eq!(workspace_a.read(&app, |ws, _| ws.tab_count()), 1); + let tab_count_b_before = workspace_b.read(&app, |ws, _| ws.tab_count()); + + app.update(|ctx| { + CrossWindowTabDrag::handle(ctx).update(ctx, |drag, _| { + drag.begin_single_tab_drag( + window_a, + Vector2F::zero(), + vec2f(800.0, 600.0), + Vector2F::zero(), + false, + vec2f(120.0, 34.0), + ); + }); + }); + + let target = AttachTarget { + window_id: window_b, + insertion_index: workspace_b.read(&app, |ws, _| ws.tab_count()), + }; + + // Must not panic. Before the fix, `on_window_transferred` synchronously + // called `old_workspace.update(...)` on window A while window A's own + // `perform_handoff` update was still on the call stack. + workspace_a.update(&mut app, |ws, ctx| { + ws.perform_handoff(target, ctx); + }); + + assert_eq!( + workspace_b.read(&app, |ws, _| ws.tab_count()), + tab_count_b_before + 1 + ); + app.read(|ctx| { + assert_eq!(SettingsPaneManager::as_ref(ctx).find_pane(window_a), None); + assert!( + SettingsPaneManager::as_ref(ctx) + .find_pane(window_b) + .is_some() + ); + }); + }); +} + +/// Regression for APP-5311 review finding #2 (important): after a transfer, +/// `open_settings_pane`'s page/search-query update must target the concrete +/// `SettingsView` embedded in the located pane, not this window's own native +/// (pre-created, non-rendered) `self.settings_pane`. +#[test] +fn test_settings_pane_page_navigation_after_transfer_updates_transferred_view() { + App::test((), |mut app| async move { + initialize_app(&mut app); + + let workspace_a = mock_workspace(&mut app); + let window_a = workspace_a.update(&mut app, |_, ctx| ctx.window_id()); + let workspace_b = mock_workspace(&mut app); + let window_b = workspace_b.update(&mut app, |_, ctx| ctx.window_id()); + + workspace_a.update(&mut app, |ws, ctx| { + ws.open_settings_pane(None, None, ctx); + }); + let settings_tab_index = workspace_a.read(&app, |ws, _| ws.tab_count() - 1); + let locator = app + .read(|ctx| SettingsPaneManager::as_ref(ctx).find_pane(window_a)) + .expect("settings pane should be registered for window A"); + + let pane_group = transfer_tab_to_new_window( + &mut app, + &workspace_a, + window_a, + &workspace_b, + window_b, + settings_tab_index, + ); + + let transferred_settings_view = pane_group.read(&app, |pane_group, ctx| { + pane_group + .downcast_pane_by_id::(locator.pane_id) + .expect("transferred pane should still be a SettingsPane") + .settings_view(ctx) + }); + let native_view_page_before = workspace_b.read(&app, |ws, ctx| { + ws.settings_pane.as_ref(ctx).current_settings_section() + }); + + workspace_b.update(&mut app, |ws, ctx| { + ws.open_settings_pane(Some(SettingsSection::Keybindings), None, ctx); + }); + + assert_eq!( + transferred_settings_view.read(&app, |view, _| view.current_settings_section()), + SettingsSection::Keybindings, + "opening settings with a page argument should navigate the transferred pane" + ); + assert_eq!( + workspace_b.read(&app, |ws, ctx| ws + .settings_pane + .as_ref(ctx) + .current_settings_section()), + native_view_page_before, + "window B's own unused native settings view must be untouched" + ); + }); +} + +/// Regression for the product decision on APP-5311: Warp enforces at most one +/// Settings pane per window. Dragging a Settings tab into a window that +/// already has one must not leave both live; the transferred one is discarded +/// and the pre-existing one is kept and remains reachable afterwards. Drives +/// the real `perform_handoff` path (not `transfer_tab_to_new_window`, whose +/// two separate top-level `app.update` calls would flush the deferred +/// reconciliation before the transferred tab is even inserted, defeating the +/// collision check). +#[test] +fn test_settings_pane_transfer_into_window_with_existing_pane_discards_duplicate() { + App::test((), |mut app| async move { + initialize_app(&mut app); + + let workspace_a = mock_workspace(&mut app); + let window_a = workspace_a.update(&mut app, |_, ctx| ctx.window_id()); + let workspace_b = mock_workspace(&mut app); + let window_b = workspace_b.update(&mut app, |_, ctx| ctx.window_id()); + + workspace_a.update(&mut app, |ws, ctx| { + ws.open_settings_pane(None, None, ctx); + }); + workspace_a.update(&mut app, |ws, ctx| { + ws.remove_tab_without_undo(0, ctx); + }); + let locator_a = app + .read(|ctx| SettingsPaneManager::as_ref(ctx).find_pane(window_a)) + .expect("settings pane should be registered for window A"); + + workspace_b.update(&mut app, |ws, ctx| { + ws.open_settings_pane(None, None, ctx); + }); + let locator_b_before = app + .read(|ctx| SettingsPaneManager::as_ref(ctx).find_pane(window_b)) + .expect("settings pane should be registered for window B"); + + app.update(|ctx| { + CrossWindowTabDrag::handle(ctx).update(ctx, |drag, _| { + drag.begin_single_tab_drag( + window_a, + Vector2F::zero(), + vec2f(800.0, 600.0), + Vector2F::zero(), + false, + vec2f(120.0, 34.0), + ); + }); + }); + let target = AttachTarget { + window_id: window_b, + insertion_index: workspace_b.read(&app, |ws, _| ws.tab_count()), + }; + workspace_a.update(&mut app, |ws, ctx| { + ws.perform_handoff(target, ctx); + }); + + // No duplicate survives: the manager still tracks (and only tracks) + // the pane that was already in window B. + app.read(|ctx| { + assert_eq!(SettingsPaneManager::as_ref(ctx).find_pane(window_a), None); + assert_eq!( + SettingsPaneManager::as_ref(ctx).find_pane(window_b), + Some(locator_b_before) + ); + }); + assert!( + workspace_b.read(&app, |ws, ctx| ws + .live_settings_view_for_locator(locator_a, ctx) + .is_none()), + "the transferred duplicate Settings pane must not still be live in window B" + ); + assert!( + workspace_b.read(&app, |ws, ctx| ws + .live_settings_view_for_locator(locator_b_before, ctx) + .is_some()), + "the pre-existing Settings pane must remain reachable" + ); + + // The surviving pane must still be reachable, and closing it must + // still let a fresh Settings tab open afterwards. + let b_settings_tab_index = workspace_b.read(&app, |ws, _| { + ws.tabs + .iter() + .position(|tab| tab.pane_group.id() == locator_b_before.pane_group_id) + .expect("surviving settings tab should still be present") + }); + workspace_b.update(&mut app, |ws, ctx| { + ws.remove_tab(b_settings_tab_index, false, true, ctx); + }); + app.read(|ctx| { + assert_eq!(SettingsPaneManager::as_ref(ctx).find_pane(window_b), None); + }); + + let tab_count_before_reopen = workspace_b.read(&app, |ws, _| ws.tab_count()); + workspace_b.update(&mut app, |ws, ctx| { + ws.open_settings_pane(None, None, ctx); + }); + assert_eq!( + workspace_b.read(&app, |ws, _| ws.tab_count()), + tab_count_before_reopen + 1 + ); + }); +} + +/// Same collision reconciliation as above, for the AI fact (Rules) pane. +#[test] +fn test_ai_fact_pane_transfer_into_window_with_existing_pane_discards_duplicate() { + App::test((), |mut app| async move { + initialize_app(&mut app); + + let workspace_a = mock_workspace(&mut app); + let window_a = workspace_a.update(&mut app, |_, ctx| ctx.window_id()); + let workspace_b = mock_workspace(&mut app); + let window_b = workspace_b.update(&mut app, |_, ctx| ctx.window_id()); + + // Give window A a second tab to host the Rules pane as a split, then + // drop the original tab so the Rules tab becomes window A's sole tab. + workspace_a.update(&mut app, |ws, ctx| { + ws.add_terminal_tab(false, ctx); + }); + workspace_a.update(&mut app, |ws, ctx| { + ws.open_ai_fact_collection_pane(Some(Direction::Right), None, ctx); + }); + workspace_a.update(&mut app, |ws, ctx| { + ws.remove_tab_without_undo(0, ctx); + }); + assert_eq!(workspace_a.read(&app, |ws, _| ws.tab_count()), 1); + let locator_a = app + .read(|ctx| AIFactManager::as_ref(ctx).find_pane(window_a)) + .expect("AI fact pane should be registered for window A"); + + workspace_b.update(&mut app, |ws, ctx| { + ws.open_ai_fact_collection_pane(Some(Direction::Right), None, ctx); + }); + let locator_b_before = app + .read(|ctx| AIFactManager::as_ref(ctx).find_pane(window_b)) + .expect("AI fact pane should be registered for window B"); + + app.update(|ctx| { + CrossWindowTabDrag::handle(ctx).update(ctx, |drag, _| { + drag.begin_single_tab_drag( + window_a, + Vector2F::zero(), + vec2f(800.0, 600.0), + Vector2F::zero(), + false, + vec2f(120.0, 34.0), + ); + }); + }); + let target = AttachTarget { + window_id: window_b, + insertion_index: workspace_b.read(&app, |ws, _| ws.tab_count()), + }; + workspace_a.update(&mut app, |ws, ctx| { + ws.perform_handoff(target, ctx); + }); + + app.read(|ctx| { + assert_eq!(AIFactManager::as_ref(ctx).find_pane(window_a), None); + assert_eq!( + AIFactManager::as_ref(ctx).find_pane(window_b), + Some(locator_b_before) + ); + }); + assert!( + workspace_b.read(&app, |ws, ctx| ws + .live_ai_fact_view_for_locator(locator_a, ctx) + .is_none()), + "the transferred duplicate Rules pane must not still be live in window B" + ); + assert!( + workspace_b.read(&app, |ws, ctx| ws + .live_ai_fact_view_for_locator(locator_b_before, ctx) + .is_some()), + "the pre-existing Rules pane must remain reachable" + ); + + let b_tab_index = workspace_b.read(&app, |ws, _| { + ws.tabs + .iter() + .position(|tab| tab.pane_group.id() == locator_b_before.pane_group_id) + .expect("surviving AI fact tab should still be present") + }); + workspace_b.update(&mut app, |ws, ctx| { + ws.remove_tab(b_tab_index, false, true, ctx); + }); + app.read(|ctx| { + assert_eq!(AIFactManager::as_ref(ctx).find_pane(window_b), None); + }); + + workspace_b.update(&mut app, |ws, ctx| { + ws.open_ai_fact_collection_pane(Some(Direction::Right), None, ctx); + }); + app.read(|ctx| { + assert!(AIFactManager::as_ref(ctx).find_pane(window_b).is_some()); + }); + }); +} From 913a1681e804990841f15f4e62832a91c507a802 Mon Sep 17 00:00:00 2001 From: Oz Date: Wed, 12 Aug 2026 06:04:20 +0000 Subject: [PATCH 3/4] Fix dangling native Settings/Rules view after cross-window transfer Workspace.settings_pane / Workspace.ai_fact_view are not private per-workspace views: SettingsPane::new / AIFactPane::new always fetch the window's registered singleton from SettingsPaneManager/ AIFactManager, so the pane's embedded view *is* that singleton. AppContext::transfer_view_tree_to_window physically relocates the entire transferred pane's view subtree -- including that view -- to the destination window, so the source window's own field and manager registration were left holding a handle to a view that no longer lived there. ViewHandle::window_id falls back to the view's *original* creation window once the current window/view_to_window mapping is gone (e.g. after the destination window closes), so open_settings_pane and open_ai_fact_collection_pane's fallback path -- which the previous commit added to stop the silent no-op -- ended up dereferencing that dangling handle back in the source window and panicking with "Circular view update" (write path) or "circular view reference" (read path via AIFactPane::from_view). Fixed by having the source workspace build itself a fresh native Settings/Rules view the moment the old one transfers out, in PaneGroup::rehome_pane_event_subscription (same self-targeted deferred dispatch already used for the subscription re-homing, so this also never touches a pane group that's still mid-update). Added Workspace::replace_native_settings_view / replace_native_ai_fact_view, factored out of the existing view-construction helpers, and two regression tests proving the native view is swapped for a fresh one and that reopening Settings/Rules afterward uses the replacement rather than the transferred view. Co-Authored-By: Warp Agent --- app/src/pane_group/mod.rs | 24 ++++- app/src/workspace/view.rs | 63 ++++++++++-- app/src/workspace/view_tests.rs | 168 ++++++++++++++++++++++++++++++++ 3 files changed, 244 insertions(+), 11 deletions(-) diff --git a/app/src/pane_group/mod.rs b/app/src/pane_group/mod.rs index b36bd8afb1e..95417196f82 100644 --- a/app/src/pane_group/mod.rs +++ b/app/src/pane_group/mod.rs @@ -8416,8 +8416,22 @@ impl PaneGroup { if let Some(old_workspace) = workspace::WorkspaceRegistry::as_ref(ctx).get(*old_window_id, ctx) { - old_workspace.update(ctx, |_, ctx| { + old_workspace.update(ctx, |old_workspace, ctx| { ctx.unsubscribe_to_view(settings_view); + // `settings_view` is the source window's native, + // per-window singleton Settings view (see + // `Workspace::settings_pane`), not a view created + // just for this one pane -- `SettingsPane::new` + // always fetches it from `SettingsPaneManager`. The + // low-level view-tree transfer that already ran + // physically relocated it to `new_window_id` + // regardless of the collision outcome above, so the + // source window's own field/registration are now + // dangling handles unless replaced here. Without + // this, the next `open_settings_pane` in this + // window panics dereferencing the relocated (or by + // then torn-down) view. See APP-5311. + old_workspace.replace_native_settings_view(ctx); }); } if let Some(new_workspace) = @@ -8450,8 +8464,14 @@ impl PaneGroup { if let Some(old_workspace) = workspace::WorkspaceRegistry::as_ref(ctx).get(*old_window_id, ctx) { - old_workspace.update(ctx, |_, ctx| { + old_workspace.update(ctx, |old_workspace, ctx| { ctx.unsubscribe_to_view(ai_fact_view); + // Same reasoning as the Settings case above: + // `ai_fact_view` is this window's native, per-window + // singleton Rules view, and it just physically + // relocated to `new_window_id` along with the + // transferred pane. See APP-5311. + old_workspace.replace_native_ai_fact_view(ctx); }); } if let Some(new_workspace) = diff --git a/app/src/workspace/view.rs b/app/src/workspace/view.rs index e007bccff38..33ee8bc83f1 100644 --- a/app/src/workspace/view.rs +++ b/app/src/workspace/view.rs @@ -1774,6 +1774,16 @@ impl Workspace { me.handle_theme_chooser_event(event, ctx); }); + let settings_pane = Self::build_native_settings_view(ctx); + + (settings_pane, theme_chooser_view) + } + + /// Creates a new native `SettingsView` for this window and registers it + /// with `SettingsPaneManager`. Factored out of `build_settings_views` so + /// `replace_native_settings_view` can create a replacement using the + /// exact same setup. + fn build_native_settings_view(ctx: &mut ViewContext) -> ViewHandle { let settings_pane = ctx.add_typed_action_view(move |ctx| SettingsView::new(None, ctx)); ctx.subscribe_to_view(&settings_pane, move |me, _, event, ctx| { me.handle_settings_pane_event(event, ctx); @@ -1784,7 +1794,49 @@ impl Workspace { manager.register_view(window_id, settings_pane.clone()); }); - (settings_pane, theme_chooser_view) + settings_pane + } + + /// Replaces this window's native Settings view (`self.settings_pane`) + /// and its `SettingsPaneManager` registration with a freshly created + /// one. `SettingsPane::new` always fetches this window's registered + /// native view rather than owning a dedicated one, so when a Settings + /// pane is dragged into another window, `AppContext:: + /// transfer_view_tree_to_window` physically relocates that same native + /// view along with it -- leaving this window's field/registration + /// dangling. Called from `PaneGroup::rehome_pane_event_subscription` + /// whenever that happens, so `self.settings_pane` and the manager + /// always point at a view that actually lives in this window. See + /// APP-5311. + pub(crate) fn replace_native_settings_view(&mut self, ctx: &mut ViewContext) { + self.settings_pane = Self::build_native_settings_view(ctx); + } + + /// Creates a new native `AIFactView` (Rules) for this window and + /// registers it with `AIFactManager`. Factored out so + /// `replace_native_ai_fact_view` can create a replacement using the + /// exact same setup. + fn build_native_ai_fact_view(ctx: &mut ViewContext) -> ViewHandle { + let ai_fact_view = ctx.add_typed_action_view(AIFactView::new); + ctx.subscribe_to_view(&ai_fact_view, move |me, _, event, ctx| { + me.handle_ai_fact_view_event(event, ctx); + }); + + let window_id = ctx.window_id(); + AIFactManager::handle(ctx).update(ctx, |manager, _| { + manager.register_view(window_id, ai_fact_view.clone()); + }); + + ai_fact_view + } + + /// Replaces this window's native AI-fact (Rules) view + /// (`self.ai_fact_view`) and its `AIFactManager` registration with a + /// freshly created one. Same reasoning as + /// `replace_native_settings_view`, for the Rules pane's per-window + /// singleton view. See APP-5311. + pub(crate) fn replace_native_ai_fact_view(&mut self, ctx: &mut ViewContext) { + self.ai_fact_view = Self::build_native_ai_fact_view(ctx); } fn build_require_login_modal(ctx: &mut ViewContext) -> ViewHandle { @@ -3047,14 +3099,7 @@ impl Workspace { me.handle_command_search_event(event, ctx); }); - let ai_fact_view = ctx.add_typed_action_view(AIFactView::new); - ctx.subscribe_to_view(&ai_fact_view, move |me, _, event, ctx| { - me.handle_ai_fact_view_event(event, ctx); - }); - - AIFactManager::handle(ctx).update(ctx, |manager, _| { - manager.register_view(window_id, ai_fact_view.clone()); - }); + let ai_fact_view = Self::build_native_ai_fact_view(ctx); let working_directories_model = ctx.add_model(|_| pane_group::WorkingDirectoriesModel::new()); diff --git a/app/src/workspace/view_tests.rs b/app/src/workspace/view_tests.rs index e72ed601473..d209760d55e 100644 --- a/app/src/workspace/view_tests.rs +++ b/app/src/workspace/view_tests.rs @@ -5411,3 +5411,171 @@ fn test_ai_fact_pane_transfer_into_window_with_existing_pane_discards_duplicate( }); }); } + +/// Regression for the crash found by adversarial verification of APP-5311: +/// `Workspace.settings_pane` is not a private per-workspace view -- it is the +/// exact same per-window singleton `SettingsPane::new` fetches from +/// `SettingsPaneManager`, so a cross-window transfer physically relocates it +/// along with the pane. Once the destination window later closes, `ViewHandle:: +/// window_id` falls back to the *original* creation window (the window- +/// removal path clears the live `view_to_window` mapping -- see +/// `ViewHandle::window_id`'s doc comment), so `open_settings_pane`'s fallback +/// path unconditionally dereferencing `self.settings_pane` would panic with +/// "Circular view update": the source window's own `views` map never held +/// that (now long gone) view. Fixed by having the source workspace build +/// itself a brand new native `SettingsView` the moment the old one transfers +/// out, rather than waiting until reopen time to discover the handle is +/// stale. This test verifies that replacement happens and that the freshly +/// opened tab after the transfer uses it, not the transferred view. +#[test] +fn test_settings_pane_native_view_is_replaced_after_transferring_out() { + App::test((), |mut app| async move { + initialize_app(&mut app); + + let workspace_a = mock_workspace(&mut app); + let window_a = workspace_a.update(&mut app, |_, ctx| ctx.window_id()); + let workspace_b = mock_workspace(&mut app); + let window_b = workspace_b.update(&mut app, |_, ctx| ctx.window_id()); + + let native_view_before_transfer = workspace_a.read(&app, |ws, _| ws.settings_pane.clone()); + + workspace_a.update(&mut app, |ws, ctx| { + ws.open_settings_pane(None, None, ctx); + }); + // The tab we just opened must be backed by window A's (pre-transfer) + // native view, confirming the premise of this test. + assert_eq!( + app.read(|ctx| SettingsPaneManager::as_ref(ctx).settings_view(window_a)), + native_view_before_transfer, + "a freshly opened Settings tab should use this window's native view" + ); + let settings_tab_index = workspace_a.read(&app, |ws, _| ws.tab_count() - 1); + + transfer_tab_to_new_window( + &mut app, + &workspace_a, + window_a, + &workspace_b, + window_b, + settings_tab_index, + ); + + // Window A must have replaced its native view with a fresh one -- + // the pre-transfer instance now lives in window B, so window A's own + // field/registration must not still reference it. + let native_view_after_transfer = workspace_a.read(&app, |ws, _| ws.settings_pane.clone()); + assert_ne!( + native_view_after_transfer, native_view_before_transfer, + "window A's native settings view should be replaced once the old one transfers out" + ); + assert_eq!( + app.read(|ctx| SettingsPaneManager::as_ref(ctx).settings_view(window_a)), + native_view_after_transfer, + "SettingsPaneManager's registration for window A must match the replacement" + ); + + // Reopening Settings in window A must create a new tab backed by the + // replacement view, not the transferred one. + let tab_count_before_reopen = workspace_a.read(&app, |ws, _| ws.tab_count()); + workspace_a.update(&mut app, |ws, ctx| { + ws.open_settings_pane(None, None, ctx); + }); + assert_eq!( + workspace_a.read(&app, |ws, _| ws.tab_count()), + tab_count_before_reopen + 1 + ); + let reopened_settings_view = workspace_a.read(&app, |ws, ctx| { + ws.active_tab_pane_group() + .as_ref(ctx) + .downcast_pane_by_id::( + SettingsPaneManager::as_ref(ctx) + .find_pane(window_a) + .expect("settings pane should be registered for window A") + .pane_id, + ) + .expect("active pane should be the reopened SettingsPane") + .settings_view(ctx) + }); + assert_eq!( + reopened_settings_view, native_view_after_transfer, + "reopened Settings tab should use window A's replacement view" + ); + assert_ne!( + reopened_settings_view, native_view_before_transfer, + "reopened Settings tab must not reference the transferred (relocated) view" + ); + }); +} + +/// Same replacement-on-transfer regression as above, for the AI fact (Rules) +/// pane's `Workspace.ai_fact_view` singleton. +#[test] +fn test_ai_fact_native_view_is_replaced_after_transferring_out() { + App::test((), |mut app| async move { + initialize_app(&mut app); + + let workspace_a = mock_workspace(&mut app); + let window_a = workspace_a.update(&mut app, |_, ctx| ctx.window_id()); + let workspace_b = mock_workspace(&mut app); + let window_b = workspace_b.update(&mut app, |_, ctx| ctx.window_id()); + + let native_view_before_transfer = workspace_a.read(&app, |ws, _| ws.ai_fact_view.clone()); + + // Give window A a second tab so `transfer_tab_to_new_window` (which + // requires more than one tab, mirroring the production drag-and-drop + // path) can transfer the Rules tab out from under it. + workspace_a.update(&mut app, |ws, ctx| { + ws.add_terminal_tab(false, ctx); + }); + workspace_a.update(&mut app, |ws, ctx| { + ws.open_ai_fact_collection_pane(Some(Direction::Right), None, ctx); + }); + let settings_tab_index = workspace_a.read(&app, |ws, _| ws.tab_count() - 1); + + transfer_tab_to_new_window( + &mut app, + &workspace_a, + window_a, + &workspace_b, + window_b, + settings_tab_index, + ); + + let native_view_after_transfer = workspace_a.read(&app, |ws, _| ws.ai_fact_view.clone()); + assert_ne!( + native_view_after_transfer, native_view_before_transfer, + "window A's native Rules view should be replaced once the old one transfers out" + ); + assert_eq!( + app.read(|ctx| AIFactManager::as_ref(ctx).ai_fact_view(window_a)), + native_view_after_transfer, + "AIFactManager's registration for window A must match the replacement" + ); + + // Reopening Rules in window A must create a new pane backed by the + // replacement view, not the transferred one. + workspace_a.update(&mut app, |ws, ctx| { + ws.open_ai_fact_collection_pane(Some(Direction::Right), None, ctx); + }); + let reopened_ai_fact_view = workspace_a.read(&app, |ws, ctx| { + ws.active_tab_pane_group() + .as_ref(ctx) + .downcast_pane_by_id::( + AIFactManager::as_ref(ctx) + .find_pane(window_a) + .expect("AI fact pane should be registered for window A") + .pane_id, + ) + .expect("active pane should be the reopened AIFactPane") + .ai_fact_view(ctx) + }); + assert_eq!( + reopened_ai_fact_view, native_view_after_transfer, + "reopened Rules pane should use window A's replacement view" + ); + assert_ne!( + reopened_ai_fact_view, native_view_before_transfer, + "reopened Rules pane must not reference the transferred (relocated) view" + ); + }); +} From 9f3ee915c6d9dd40d822411180c59753f0d013de Mon Sep 17 00:00:00 2001 From: Oz Date: Wed, 12 Aug 2026 06:29:30 +0000 Subject: [PATCH 4/4] Add regression test for APP-5311 cross-window Settings/Rules drag Drives the real cross-window tab-drag gesture (real mouse events, never the internal transfer APIs) to cover all three reported symptoms plus the one-pane-per-window collision case: - Flow A: Settings reopens in the original window after being dragged into a new window and that window is closed. - Flow B: clicking Rules from a Settings pane hosted in another window opens Rules in that same window, not the original one. - Flow C: after Settings+Rules are dragged into a window and Rules is closed there, clicking Rules again reopens it in that window. - Collision: dragging a Settings tab into a window that already has its own Settings pane keeps a single pane, not two. Registered as a manual/ignored nextest test (real windowing + drag feature required) and in the standalone integration test binary, with video recording wired via with_start_recording()/with_stop_recording(). Co-Authored-By: Warp Agent --- crates/integration/src/bin/integration.rs | 1 + crates/integration/src/test/workspace.rs | 449 +++++++++++++++++- .../integration/tests/integration/ui_tests.rs | 5 + 3 files changed, 453 insertions(+), 2 deletions(-) diff --git a/crates/integration/src/bin/integration.rs b/crates/integration/src/bin/integration.rs index 8e1e4ee7673..7653a29fe3d 100644 --- a/crates/integration/src/bin/integration.rs +++ b/crates/integration/src/bin/integration.rs @@ -371,6 +371,7 @@ fn register_tests() -> HashMap<&'static str, BoxedBuilderFn> { register_test!(test_attach_tab_to_other_window_and_continue_drag); register_test!(test_single_tab_handoff_continues_drag); register_test!(test_multi_tab_drag_back_to_source_and_out_again); + register_test!(test_settings_and_rules_panes_survive_cross_window_drag); register_test!(test_restore_single_closed_pane); register_test!(test_restore_multiple_closed_panes); diff --git a/crates/integration/src/test/workspace.rs b/crates/integration/src/test/workspace.rs index 51046486647..67e9a9c54b6 100644 --- a/crates/integration/src/test/workspace.rs +++ b/crates/integration/src/test/workspace.rs @@ -9,7 +9,9 @@ use settings::Setting as _; use warp::cmd_or_ctrl_shift; use warp::features::FeatureFlag; use warp::integration_testing::clipboard::assert_clipboard_contains_string; -use warp::integration_testing::pane_group::assert_focused_pane_index; +use warp::integration_testing::pane_group::{ + assert_focused_pane_index, assert_num_panes_in_tab, close_pane_by_index, +}; use warp::integration_testing::step::new_step_with_default_assertions; use warp::integration_testing::terminal::util::{ ExpectedExitStatus, current_shell_starter_and_version, @@ -20,7 +22,7 @@ use warp::integration_testing::terminal::{ execute_command, execute_command_for_single_terminal_in_tab, wait_until_bootstrapped_pane, wait_until_bootstrapped_single_pane_for_tab, }; -use warp::integration_testing::view_getters::{terminal_view, workspace_view}; +use warp::integration_testing::view_getters::{pane_group_view, terminal_view, workspace_view}; use warp::integration_testing::window::{ add_and_save_window, assert_num_windows_open, save_active_window_id, }; @@ -28,6 +30,7 @@ use warp::integration_testing::workspace::{ assert_focused_tab_index, assert_tab_count, press_native_modal_button, }; use warp::settings::PaneSettings; +use warp::settings_view::{SettingsView, SettingsViewEvent}; use warp::terminal::shell::ShellType; use warp::workspace::tab_settings::{TabSettings, VerticalTabsDisplayGranularity}; use warp::workspace::{NEW_TAB_BUTTON_POSITION_ID, WorkspaceAction}; @@ -469,6 +472,20 @@ fn drag_tabs_feature_enabled() -> bool { cfg!(feature = "drag_tabs_to_windows") } +/// Like the production `assert_num_panes_in_tab` helper, but checks +/// `visible_pane_count()` instead of `pane_count()`. `UndoClosedPanes` is on +/// by default, so a closed pane is hidden rather than removed from +/// `pane_count()` -- this is what a test should check after closing a pane +/// via the UI, since that's what's actually visible on screen. +fn assert_num_visible_panes_in_tab(tab_index: usize, num_panes: usize) -> AssertionCallback { + Box::new(move |app, window_id| { + let pane_group = pane_group_view(app, window_id, tab_index); + pane_group.read(app, |view, _| { + async_assert_eq!(view.visible_pane_count(), num_panes) + }) + }) +} + pub fn test_active_session_follows_focus() -> Builder { new_builder() .set_should_run_test(skip_if_powershell_core_2303) @@ -1377,3 +1394,431 @@ pub fn test_single_tab_handoff_continues_drag() -> Builder { ) .with_step(focus_saved_window(TARGET_WINDOW_KEY).add_assertion(assert_tab_count(1))) } + +/// Drags the Settings tab (always at index 1: index 0 is the terminal tab) out +/// of the saved source window via real mouse events, mirroring +/// `test_detach_tab_to_new_window_with_drag`. Used as the shared "detach" +/// gesture for the APP-5311 visual verification flows below, since the bug +/// and its fix are both specifically about the real drag-driven handoff path +/// (not the internal transfer APIs exercised by the unit tests in +/// `view_tests.rs`). +fn detach_settings_tab_step(step_name: &'static str) -> TestStep { + const SETTINGS_TAB_INDEX: usize = 1; + TestStep::new(step_name) + .with_action(|app, _, data| { + let source_window_id = *data + .get::<_, WindowId>(SOURCE_WINDOW_KEY) + .expect("saved source window id should exist"); + let start = tab_center(app, source_window_id, SETTINGS_TAB_INDEX); + dispatch_mouse_event( + app, + source_window_id, + Event::LeftMouseDown { + position: start, + modifiers: ModifiersState::default(), + click_count: 1, + is_first_mouse: false, + }, + ); + }) + .with_action(|app, _, data| { + let source_window_id = *data + .get::<_, WindowId>(SOURCE_WINDOW_KEY) + .expect("saved source window id should exist"); + let start = tab_center(app, source_window_id, SETTINGS_TAB_INDEX); + dispatch_mouse_event( + app, + source_window_id, + Event::LeftMouseDragged { + position: start + vec2f(12.0, 0.0), + modifiers: ModifiersState::default(), + }, + ); + }) + .with_action(|app, _, data| { + let source_window_id = *data + .get::<_, WindowId>(SOURCE_WINDOW_KEY) + .expect("saved source window id should exist"); + let start = tab_center(app, source_window_id, SETTINGS_TAB_INDEX); + dispatch_mouse_event( + app, + source_window_id, + Event::LeftMouseDragged { + position: start + vec2f(0.0, 140.0), + modifiers: ModifiersState::default(), + }, + ); + }) + .with_action(|app, _, data| { + let source_window_id = *data + .get::<_, WindowId>(SOURCE_WINDOW_KEY) + .expect("saved source window id should exist"); + let start = tab_center(app, source_window_id, SETTINGS_TAB_INDEX); + let drop_position = start + vec2f(220.0, 220.0); + dispatch_mouse_event( + app, + source_window_id, + Event::LeftMouseDragged { + position: drop_position, + modifiers: ModifiersState::default(), + }, + ); + }) + .with_action(|app, _, data| { + let source_window_id = *data + .get::<_, WindowId>(SOURCE_WINDOW_KEY) + .expect("saved source window id should exist"); + let start = tab_center(app, source_window_id, SETTINGS_TAB_INDEX); + let drop_position = start + vec2f(220.0, 220.0); + dispatch_mouse_event( + app, + source_window_id, + Event::LeftMouseUp { + position: drop_position, + modifiers: ModifiersState::default(), + }, + ); + }) + .add_assertion(assert_num_windows_open(2)) + .add_assertion(assert_tab_count(1)) +} + +/// Drags the source window's Settings tab (index 1) into the tab bar of the +/// window saved under `DETACHED_WINDOW_KEY`, mirroring +/// `test_attach_tab_to_other_window_and_continue_drag`. Used for the +/// one-pane-per-window collision case, where the destination window already +/// hosts its own Settings pane. +fn drag_settings_tab_into_detached_window_step() -> TestStep { + const SETTINGS_TAB_INDEX: usize = 1; + TestStep::new( + "Drag Settings from the original window into the window that already has Settings", + ) + .with_action(|app, _, data| { + let source_window_id = *data + .get::<_, WindowId>(SOURCE_WINDOW_KEY) + .expect("saved source window id should exist"); + let start = tab_center(app, source_window_id, SETTINGS_TAB_INDEX); + dispatch_mouse_event( + app, + source_window_id, + Event::LeftMouseDown { + position: start, + modifiers: ModifiersState::default(), + click_count: 1, + is_first_mouse: false, + }, + ); + }) + .with_action(|app, _, data| { + let source_window_id = *data + .get::<_, WindowId>(SOURCE_WINDOW_KEY) + .expect("saved source window id should exist"); + let start = tab_center(app, source_window_id, SETTINGS_TAB_INDEX); + dispatch_mouse_event( + app, + source_window_id, + Event::LeftMouseDragged { + position: start + vec2f(12.0, 0.0), + modifiers: ModifiersState::default(), + }, + ); + }) + .with_action(|app, _, data| { + let source_window_id = *data + .get::<_, WindowId>(SOURCE_WINDOW_KEY) + .expect("saved source window id should exist"); + let start = tab_center(app, source_window_id, SETTINGS_TAB_INDEX); + dispatch_mouse_event( + app, + source_window_id, + Event::LeftMouseDragged { + position: start + vec2f(0.0, 220.0), + modifiers: ModifiersState::default(), + }, + ); + }) + .with_action(|app, _, data| { + let source_window_id = *data + .get::<_, WindowId>(SOURCE_WINDOW_KEY) + .expect("saved source window id should exist"); + let target_window_id = *data + .get::<_, WindowId>(DETACHED_WINDOW_KEY) + .expect("saved detached window id should exist"); + let target_tab_bounds = tab_bounds(app, target_window_id, 0); + let attach_point = tab_screen_point( + app, + target_window_id, + 0, + 8.0, + target_tab_bounds.height() / 2.0, + ); + let source_local_target = + source_local_point_for_screen_point(app, source_window_id, attach_point); + dispatch_mouse_event( + app, + source_window_id, + Event::LeftMouseDragged { + position: source_local_target, + modifiers: ModifiersState::default(), + }, + ); + }) + .with_action(|app, _, data| { + let source_window_id = *data + .get::<_, WindowId>(SOURCE_WINDOW_KEY) + .expect("saved source window id should exist"); + let target_window_id = *data + .get::<_, WindowId>(DETACHED_WINDOW_KEY) + .expect("saved detached window id should exist"); + let target_tab_bounds = tab_bounds(app, target_window_id, 0); + let attach_point = tab_screen_point( + app, + target_window_id, + 0, + 8.0, + target_tab_bounds.height() / 2.0, + ); + let source_local_target = + source_local_point_for_screen_point(app, source_window_id, attach_point); + dispatch_mouse_event( + app, + source_window_id, + Event::LeftMouseDragged { + position: source_local_target, + modifiers: ModifiersState::default(), + }, + ); + }) + .with_action(|app, _, data| { + let source_window_id = *data + .get::<_, WindowId>(SOURCE_WINDOW_KEY) + .expect("saved source window id should exist"); + let target_window_id = *data + .get::<_, WindowId>(DETACHED_WINDOW_KEY) + .expect("saved detached window id should exist"); + let target_tab_bounds = tab_bounds(app, target_window_id, 0); + let attach_point = tab_screen_point( + app, + target_window_id, + 0, + 8.0, + target_tab_bounds.height() / 2.0, + ); + let source_local_target = + source_local_point_for_screen_point(app, source_window_id, attach_point); + dispatch_mouse_event( + app, + source_window_id, + Event::LeftMouseUp { + position: source_local_target, + modifiers: ModifiersState::default(), + }, + ); + }) +} + +/// Dispatches `WorkspaceAction::ShowSettings` on the workspace hosted in the +/// given saved window -- exactly the action the tab-bar gear icon's `on_click` +/// dispatches. Used instead of the `cmdorctrl-,` keybinding because keystrokes +/// depend on keyboard-focus bookkeeping that this test's synthetic window +/// juggling (detach/close/refocus) does not reliably keep current; the direct +/// action dispatch is the same real product code path (`show_settings`), not +/// a bypass of anything under test here. +fn open_settings_step(step_name: &'static str, window_key: &'static str) -> TestStep { + TestStep::new(step_name).with_action(move |app, _, data| { + let window_id = *data + .get::<_, WindowId>(window_key) + .expect("saved window id should exist"); + // Deferred rather than a direct `handle_action` call: this can run + // immediately after a cross-window transfer/close settles, and a + // synchronous call can re-enter a workspace view update still being + // flushed from that prior transfer ("Circular view update"). + workspace_view(app, window_id).update(app, |_, ctx| { + ctx.dispatch_typed_action_deferred(WorkspaceAction::ShowSettings); + }); + }) +} + +/// Emits `SettingsViewEvent::OpenAIFactCollection` on the live `SettingsView` +/// hosted in the given saved window, i.e. exactly the event +/// `ManageRulesWidget`'s "Manage rules" button dispatches on click. Used +/// instead of a raw screen click because that button has no cached click +/// position; the event itself is real product wiring, not a bypass of the +/// cross-window transfer mechanism under test (which is always exercised via +/// real mouse drag events in this file). +fn click_rules_button_in_settings_step( + step_name: &'static str, + window_key: &'static str, +) -> TestStep { + TestStep::new(step_name).with_action(move |app, _, data| { + let window_id = *data + .get::<_, WindowId>(window_key) + .expect("saved window id should exist"); + let settings_view = app + .views_of_type::(window_id) + .expect("settings view should exist in the window") + .first() + .expect("settings view should exist in the window") + .clone(); + settings_view.update(app, |_, ctx| { + ctx.emit(SettingsViewEvent::OpenAIFactCollection); + }); + }) +} + +/// Closes the saved window via `TerminationMode::Cancellable` -- the same +/// path a real click on the window's close button takes (see +/// `close_window_requested` in `crates/warpui/src/windowing/winit/event_loop/mod.rs`, +/// which falls through to this same internal close for an approved, +/// interruptible close). Used instead of the `close_window` production test +/// helper (which hardcodes `ForceTerminate`) so this test exercises the real +/// user gesture, not just a forced teardown. +fn close_window_via_real_close_button( + step_name: &'static str, + window_key: &'static str, + expected_num_windows: usize, +) -> TestStep { + new_step_with_default_assertions(step_name) + .with_action(move |app, _, data| { + let window_id = *data + .get::<_, WindowId>(window_key) + .expect("saved window id should exist"); + app.update(|ctx| { + WindowManager::as_ref(ctx).close_window( + window_id, + warpui_core::platform::TerminationMode::Cancellable, + ); + }); + }) + .add_assertion(move |app, _| async_assert_eq!(app.window_ids().len(), expected_num_windows)) +} + +/// Manual visual-verification companion for APP-5311. Drives the real +/// cross-window tab-drag gesture (never the internal transfer APIs used by +/// the unit tests in `view_tests.rs`) to exercise all three reported +/// symptoms plus the one-pane-per-window collision reconciliation: +/// +/// - Flow A: Settings reopens in the original window after being dragged +/// into a new window and that window is closed. +/// - Flow B: clicking "Rules" from a Settings pane hosted in another window +/// opens Rules in that same window, not the original one. +/// - Flow C: after Settings+Rules are dragged together into a window and +/// Rules is closed there, clicking "Rules" again reopens it in that window. +/// - Collision: dragging a Settings tab into a window that already has its +/// own Settings pane discards the duplicate and keeps a single pane. +/// +/// Requires `WARPUI_USE_REAL_DISPLAY_IN_INTEGRATION_TESTS=1` to capture +/// video/screenshots and the `drag_tabs_to_windows` cargo feature to +/// exercise the drag gesture; skipped otherwise via `drag_tabs_feature_enabled`. +pub fn test_settings_and_rules_panes_survive_cross_window_drag() -> Builder { + new_builder() + .set_should_run_test(drag_tabs_feature_enabled) + .with_step(wait_until_bootstrapped_single_pane_for_tab(0)) + .with_step( + new_step_with_default_assertions("Save the original window") + .add_assertion(save_active_window_id(SOURCE_WINDOW_KEY)) + .with_start_recording(), + ) + .with_step( + open_settings_step("Open Settings", SOURCE_WINDOW_KEY) + .add_assertion(assert_tab_count(2)), + ) + // ---- Flow A: reopen Settings after detach + close ---- + .with_step(detach_settings_tab_step( + "Flow A: drag Settings into a new window", + )) + .with_step( + focus_other_window(DETACHED_WINDOW_KEY, SOURCE_WINDOW_KEY) + .add_assertion(assert_tab_count(1)), + ) + // Focus back to the source window *before* closing the detached one: closing + // the currently-active window would leave the next step's default assertions + // (which resolve views via the active window) pointed at a window that no + // longer exists. + .with_step(focus_saved_window(SOURCE_WINDOW_KEY)) + .with_step(close_window_via_real_close_button( + "Flow A: close the detached window (real close button path)", + DETACHED_WINDOW_KEY, + 1, + )) + .with_step( + open_settings_step( + "Flow A: Settings reopens in the original window after detach + close", + SOURCE_WINDOW_KEY, + ) + .add_assertion(assert_tab_count(2)), + ) + // ---- Flow B: Rules opens in the window hosting the transferred Settings pane ---- + .with_step(detach_settings_tab_step( + "Flow B: drag Settings into a new window", + )) + .with_step( + focus_other_window(DETACHED_WINDOW_KEY, SOURCE_WINDOW_KEY) + .add_assertion(assert_tab_count(1)), + ) + .with_step( + click_rules_button_in_settings_step( + "Flow B: click Rules from the transferred Settings pane", + DETACHED_WINDOW_KEY, + ) + .add_assertion(assert_num_panes_in_tab(0, 2)), + ) + .with_step( + TestStep::new("Flow B: the original window is untouched") + .add_named_assertion_with_data_from_prior_step( + "original window's active tab pane count is unchanged", + |app, _window_id, data| { + let source_window_id = *data + .get::<_, WindowId>(SOURCE_WINDOW_KEY) + .expect("saved source window id should exist"); + let pane_group = pane_group_view(app, source_window_id, 0); + pane_group.read(app, |pane_group, _| { + async_assert_eq!(pane_group.pane_count(), 1) + }) + }, + ), + ) + // ---- Flow C: Rules reopens after being closed in the transferred window ---- + // `UndoClosedPanes` is on by default (see `undo_closed_panes` in the default + // feature list), so closing a pane hides it for undo rather than removing it + // from `pane_count()`; check `visible_pane_count()` instead. + .with_step(close_pane_by_index(0, 1).add_assertion(assert_num_visible_panes_in_tab(0, 1))) + .with_step( + click_rules_button_in_settings_step( + "Flow C: click Rules again after closing it", + DETACHED_WINDOW_KEY, + ) + .add_assertion(assert_num_visible_panes_in_tab(0, 2)), + ) + // ---- Collision: dragging Settings into a window that already has one keeps a single pane ---- + .with_step(focus_saved_window(SOURCE_WINDOW_KEY).add_assertion(assert_tab_count(1))) + .with_step( + open_settings_step( + "Open a fresh Settings pane in the original window", + SOURCE_WINDOW_KEY, + ) + .add_assertion(assert_tab_count(2)), + ) + // Windows in this harness render at a fairly large default size, so the + // separation between origins needs to exceed that size or the two + // windows' bounds overlap and the drag never actually leaves the + // source window (it gets treated as a reorder-in-place instead of a + // cross-window attach). + .with_step(set_saved_window_origin(SOURCE_WINDOW_KEY, vec2f(0.0, 0.0))) + .with_step(set_saved_window_origin( + DETACHED_WINDOW_KEY, + vec2f(2000.0, 0.0), + )) + .with_step(focus_saved_window(SOURCE_WINDOW_KEY)) + .with_step(drag_settings_tab_into_detached_window_step()) + .with_step( + focus_saved_window(DETACHED_WINDOW_KEY) + .add_assertion(assert_tab_count(1)) + .add_assertion(assert_num_visible_panes_in_tab(0, 2)), + ) + .with_step( + focus_saved_window(SOURCE_WINDOW_KEY) + .add_assertion(assert_tab_count(1)) + .with_stop_recording(), + ) +} diff --git a/crates/integration/tests/integration/ui_tests.rs b/crates/integration/tests/integration/ui_tests.rs index 104b312f93a..50fbdddded7 100644 --- a/crates/integration/tests/integration/ui_tests.rs +++ b/crates/integration/tests/integration/ui_tests.rs @@ -232,6 +232,11 @@ integration_tests! { test_attach_tab_to_other_window_and_continue_drag, test_single_tab_handoff_continues_drag, test_multi_tab_drag_back_to_source_and_out_again, + // Manual visual-verification companion for APP-5311: drives the real cross-window + // drag gesture and is meant to be run manually with WARPUI_USE_REAL_DISPLAY_IN_INTEGRATION_TESTS=1 + // to capture video/screenshots. + #[ignore = "Manual test: requires real display for video capture (APP-5311 visual verification)"] + test_settings_and_rules_panes_survive_cross_window_drag, test_restore_single_closed_pane, test_restore_multiple_closed_panes,