From caebd0cc4ddfd4a0838378d8ace632a92f682328 Mon Sep 17 00:00:00 2001 From: Anthony Eid <56899983+Anthony-Eid@users.noreply.github.com> Date: Thu, 4 Sep 2025 13:55:20 -0400 Subject: [PATCH] debugger: Fix stack frame filter crash (#37555) The crash was caused by not accounting for the fact that a range of collapse frames only counts as one entry. Causing the filter indices to overshoot for indices after collapse frames (it was counting all collapse frames instead of just one). The test missed this because it all happened in one `cx.update` closure and didn't render the stack frame list when the filter was applied. The test has been updated to account for this. Release Notes: - N/A Co-authored-by: Cole Miller --- .../src/session/running/stack_frame_list.rs | 60 ++++-- .../debugger_ui/src/tests/stack_frame_list.rs | 194 ++++++++++++------ 2 files changed, 165 insertions(+), 89 deletions(-) diff --git a/crates/debugger_ui/src/session/running/stack_frame_list.rs b/crates/debugger_ui/src/session/running/stack_frame_list.rs index f80173c365a047da39733c94964c473bef579e1c..e51b8da362a581c96d2872a213a8be32ff31b097 100644 --- a/crates/debugger_ui/src/session/running/stack_frame_list.rs +++ b/crates/debugger_ui/src/session/running/stack_frame_list.rs @@ -28,8 +28,8 @@ pub enum StackFrameListEvent { } /// Represents the filter applied to the stack frame list -#[derive(PartialEq, Eq, Copy, Clone)] -enum StackFrameFilter { +#[derive(PartialEq, Eq, Copy, Clone, Debug)] +pub(crate) enum StackFrameFilter { /// Show all frames All, /// Show only frames from the user's code @@ -174,19 +174,29 @@ impl StackFrameList { #[cfg(test)] pub(crate) fn dap_stack_frames(&self, cx: &mut App) -> Vec { - self.stack_frames(cx) - .unwrap_or_default() - .into_iter() - .enumerate() - .filter(|(ix, _)| { - self.list_filter == StackFrameFilter::All - || self - .filter_entries_indices - .binary_search_by_key(&ix, |ix| ix) - .is_ok() - }) - .map(|(_, stack_frame)| stack_frame.dap) - .collect() + match self.list_filter { + StackFrameFilter::All => self + .stack_frames(cx) + .unwrap_or_default() + .into_iter() + .map(|stack_frame| stack_frame.dap) + .collect(), + StackFrameFilter::OnlyUserFrames => self + .filter_entries_indices + .iter() + .map(|ix| match &self.entries[*ix] { + StackFrameEntry::Label(label) => label, + StackFrameEntry::Collapsed(_) => panic!("Collapsed tabs should not be visible"), + StackFrameEntry::Normal(frame) => frame, + }) + .cloned() + .collect(), + } + } + + #[cfg(test)] + pub(crate) fn list_filter(&self) -> StackFrameFilter { + self.list_filter } pub fn opened_stack_frame_id(&self) -> Option { @@ -246,6 +256,7 @@ impl StackFrameList { self.entries.clear(); self.selected_ix = None; self.list_state.reset(0); + self.filter_entries_indices.clear(); cx.emit(StackFrameListEvent::BuiltEntries); cx.notify(); return; @@ -263,7 +274,7 @@ impl StackFrameList { .unwrap_or_default(); let mut filter_entries_indices = Vec::default(); - for (ix, stack_frame) in stack_frames.iter().enumerate() { + for stack_frame in stack_frames.iter() { let frame_in_visible_worktree = stack_frame.dap.source.as_ref().is_some_and(|source| { source.path.as_ref().is_some_and(|path| { worktree_prefixes @@ -273,10 +284,6 @@ impl StackFrameList { }) }); - if frame_in_visible_worktree { - filter_entries_indices.push(ix); - } - match stack_frame.dap.presentation_hint { Some(dap::StackFramePresentationHint::Deemphasize) | Some(dap::StackFramePresentationHint::Subtle) => { @@ -302,6 +309,9 @@ impl StackFrameList { first_stack_frame_with_path.get_or_insert(entries.len()); } entries.push(StackFrameEntry::Normal(stack_frame.dap.clone())); + if frame_in_visible_worktree { + filter_entries_indices.push(entries.len() - 1); + } } } } @@ -309,7 +319,6 @@ impl StackFrameList { let collapsed_entries = std::mem::take(&mut collapsed_entries); if !collapsed_entries.is_empty() { entries.push(StackFrameEntry::Collapsed(collapsed_entries)); - self.filter_entries_indices.push(entries.len() - 1); } self.entries = entries; self.filter_entries_indices = filter_entries_indices; @@ -612,7 +621,16 @@ impl StackFrameList { let entries = std::mem::take(stack_frames) .into_iter() .map(StackFrameEntry::Normal); + // HERE + let entries_len = entries.len(); self.entries.splice(ix..ix + 1, entries); + let (Ok(filtered_indices_start) | Err(filtered_indices_start)) = + self.filter_entries_indices.binary_search(&ix); + + for idx in &mut self.filter_entries_indices[filtered_indices_start..] { + *idx += entries_len - 1; + } + self.selected_ix = Some(ix); self.list_state.reset(self.entries.len()); cx.emit(StackFrameListEvent::BuiltEntries); diff --git a/crates/debugger_ui/src/tests/stack_frame_list.rs b/crates/debugger_ui/src/tests/stack_frame_list.rs index 023056224e177bb053f5188ced59c059c9c8ad32..a61a31d270c9d599f30185d7da3c825c51bb7898 100644 --- a/crates/debugger_ui/src/tests/stack_frame_list.rs +++ b/crates/debugger_ui/src/tests/stack_frame_list.rs @@ -1,6 +1,6 @@ use crate::{ debugger_panel::DebugPanel, - session::running::stack_frame_list::StackFrameEntry, + session::running::stack_frame_list::{StackFrameEntry, StackFrameFilter}, tests::{active_debug_session_panel, init_test, init_test_workspace, start_debug_session}, }; use dap::{ @@ -867,6 +867,28 @@ async fn test_stack_frame_filter(executor: BackgroundExecutor, cx: &mut TestAppC }, StackFrame { id: 4, + name: "node:internal/modules/run_main2".into(), + source: Some(dap::Source { + name: Some("run_main.js".into()), + path: Some(path!("/usr/lib/node/internal/modules/run_main2.js").into()), + source_reference: None, + presentation_hint: None, + origin: None, + sources: None, + adapter_data: None, + checksums: None, + }), + line: 50, + column: 1, + end_line: None, + end_column: None, + can_restart: None, + instruction_pointer_reference: None, + module_id: None, + presentation_hint: Some(dap::StackFramePresentationHint::Deemphasize), + }, + StackFrame { + id: 5, name: "doSomething".into(), source: Some(dap::Source { name: Some("test.js".into()), @@ -957,83 +979,119 @@ async fn test_stack_frame_filter(executor: BackgroundExecutor, cx: &mut TestAppC cx.run_until_parked(); - active_debug_session_panel(workspace, cx).update_in(cx, |debug_panel_item, window, cx| { - let stack_frame_list = debug_panel_item - .running_state() - .update(cx, |state, _| state.stack_frame_list().clone()); + let stack_frame_list = + active_debug_session_panel(workspace, cx).update_in(cx, |debug_panel_item, window, cx| { + let stack_frame_list = debug_panel_item + .running_state() + .update(cx, |state, _| state.stack_frame_list().clone()); + + stack_frame_list.update(cx, |stack_frame_list, cx| { + stack_frame_list.build_entries(true, window, cx); + + // Verify we have the expected collapsed structure + assert_eq!( + stack_frame_list.entries(), + &vec![ + StackFrameEntry::Normal(stack_frames_for_assertions[0].clone()), + StackFrameEntry::Collapsed(vec![ + stack_frames_for_assertions[1].clone(), + stack_frames_for_assertions[2].clone(), + stack_frames_for_assertions[3].clone() + ]), + StackFrameEntry::Normal(stack_frames_for_assertions[4].clone()), + ] + ); + }); - stack_frame_list.update(cx, |stack_frame_list, cx| { - stack_frame_list.build_entries(true, window, cx); + stack_frame_list + }); - // Verify we have the expected collapsed structure - assert_eq!( - stack_frame_list.entries(), - &vec![ - StackFrameEntry::Normal(stack_frames_for_assertions[0].clone()), - StackFrameEntry::Collapsed(vec![ - stack_frames_for_assertions[1].clone(), - stack_frames_for_assertions[2].clone() - ]), - StackFrameEntry::Normal(stack_frames_for_assertions[3].clone()), - ] - ); + stack_frame_list.update(cx, |stack_frame_list, cx| { + let all_frames = stack_frame_list.flatten_entries(true, false); + assert_eq!(all_frames.len(), 5, "Should see all 5 frames initially"); - // Test 1: Verify filtering works - let all_frames = stack_frame_list.flatten_entries(true, false); - assert_eq!(all_frames.len(), 4, "Should see all 4 frames initially"); + stack_frame_list + .toggle_frame_filter(Some(project::debugger::session::ThreadStatus::Stopped), cx); + assert_eq!( + stack_frame_list.list_filter(), + StackFrameFilter::OnlyUserFrames + ); + }); - // Toggle to user frames only - stack_frame_list - .toggle_frame_filter(Some(project::debugger::session::ThreadStatus::Stopped), cx); + stack_frame_list.update(cx, |stack_frame_list, cx| { + let user_frames = stack_frame_list.dap_stack_frames(cx); + assert_eq!(user_frames.len(), 2, "Should only see 2 user frames"); + assert_eq!(user_frames[0].name, "main"); + assert_eq!(user_frames[1].name, "doSomething"); + + // Toggle back to all frames + stack_frame_list + .toggle_frame_filter(Some(project::debugger::session::ThreadStatus::Stopped), cx); + assert_eq!(stack_frame_list.list_filter(), StackFrameFilter::All); + }); - let user_frames = stack_frame_list.dap_stack_frames(cx); - assert_eq!(user_frames.len(), 2, "Should only see 2 user frames"); - assert_eq!(user_frames[0].name, "main"); - assert_eq!(user_frames[1].name, "doSomething"); + stack_frame_list.update(cx, |stack_frame_list, cx| { + let all_frames_again = stack_frame_list.flatten_entries(true, false); + assert_eq!( + all_frames_again.len(), + 5, + "Should see all 5 frames after toggling back" + ); - // Test 2: Verify filtering toggles correctly - // Check we can toggle back and see all frames again + // Test 3: Verify collapsed entries stay expanded + stack_frame_list.expand_collapsed_entry(1, cx); + assert_eq!( + stack_frame_list.entries(), + &vec![ + StackFrameEntry::Normal(stack_frames_for_assertions[0].clone()), + StackFrameEntry::Normal(stack_frames_for_assertions[1].clone()), + StackFrameEntry::Normal(stack_frames_for_assertions[2].clone()), + StackFrameEntry::Normal(stack_frames_for_assertions[3].clone()), + StackFrameEntry::Normal(stack_frames_for_assertions[4].clone()), + ] + ); - // Toggle back to all frames - stack_frame_list - .toggle_frame_filter(Some(project::debugger::session::ThreadStatus::Stopped), cx); + stack_frame_list + .toggle_frame_filter(Some(project::debugger::session::ThreadStatus::Stopped), cx); + assert_eq!( + stack_frame_list.list_filter(), + StackFrameFilter::OnlyUserFrames + ); + }); - let all_frames_again = stack_frame_list.flatten_entries(true, false); - assert_eq!( - all_frames_again.len(), - 4, - "Should see all 4 frames after toggling back" - ); + stack_frame_list.update(cx, |stack_frame_list, cx| { + stack_frame_list + .toggle_frame_filter(Some(project::debugger::session::ThreadStatus::Stopped), cx); + assert_eq!(stack_frame_list.list_filter(), StackFrameFilter::All); + }); - // Test 3: Verify collapsed entries stay expanded - stack_frame_list.expand_collapsed_entry(1, cx); - assert_eq!( - stack_frame_list.entries(), - &vec![ - StackFrameEntry::Normal(stack_frames_for_assertions[0].clone()), - StackFrameEntry::Normal(stack_frames_for_assertions[1].clone()), - StackFrameEntry::Normal(stack_frames_for_assertions[2].clone()), - StackFrameEntry::Normal(stack_frames_for_assertions[3].clone()), - ] - ); + stack_frame_list.update(cx, |stack_frame_list, cx| { + stack_frame_list + .toggle_frame_filter(Some(project::debugger::session::ThreadStatus::Stopped), cx); + assert_eq!( + stack_frame_list.list_filter(), + StackFrameFilter::OnlyUserFrames + ); - // Toggle filter twice - stack_frame_list - .toggle_frame_filter(Some(project::debugger::session::ThreadStatus::Stopped), cx); - stack_frame_list - .toggle_frame_filter(Some(project::debugger::session::ThreadStatus::Stopped), cx); + assert_eq!( + stack_frame_list.dap_stack_frames(cx).as_slice(), + &[ + stack_frames_for_assertions[0].clone(), + stack_frames_for_assertions[4].clone() + ] + ); - // Verify entries remain expanded - assert_eq!( - stack_frame_list.entries(), - &vec![ - StackFrameEntry::Normal(stack_frames_for_assertions[0].clone()), - StackFrameEntry::Normal(stack_frames_for_assertions[1].clone()), - StackFrameEntry::Normal(stack_frames_for_assertions[2].clone()), - StackFrameEntry::Normal(stack_frames_for_assertions[3].clone()), - ], - "Expanded entries should remain expanded after toggling filter" - ); - }); + // Verify entries remain expanded + assert_eq!( + stack_frame_list.entries(), + &vec![ + StackFrameEntry::Normal(stack_frames_for_assertions[0].clone()), + StackFrameEntry::Normal(stack_frames_for_assertions[1].clone()), + StackFrameEntry::Normal(stack_frames_for_assertions[2].clone()), + StackFrameEntry::Normal(stack_frames_for_assertions[3].clone()), + StackFrameEntry::Normal(stack_frames_for_assertions[4].clone()), + ], + "Expanded entries should remain expanded after toggling filter" + ); }); }