debugger: Fix stack frame filter crash (#37555)

Anthony Eid and Cole Miller created

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 <cole@zed.dev>

Change summary

crates/debugger_ui/src/session/running/stack_frame_list.rs |  60 +
crates/debugger_ui/src/tests/stack_frame_list.rs           | 194 +++++--
2 files changed, 165 insertions(+), 89 deletions(-)

Detailed changes

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<dap::StackFrame> {
-        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<StackFrameId> {
@@ -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);

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"
+        );
     });
 }