diff --git a/crates/sidebar/src/sidebar.rs b/crates/sidebar/src/sidebar.rs index 3cd7e0059ac165bcd5e738591363cb600abcd60f..c49e4a91ef6793f8ff3bfedd656d33a89b305542 100644 --- a/crates/sidebar/src/sidebar.rs +++ b/crates/sidebar/src/sidebar.rs @@ -32,7 +32,8 @@ use ui::utils::platform_title_bar_height; use serde::{Deserialize, Serialize}; use settings::Settings as _; -use std::collections::{HashMap, HashSet}; +use std::collections::{HashMap, HashSet, VecDeque}; +use std::hash::{Hash, Hasher}; use std::mem; use std::path::PathBuf; use std::rc::Rc; @@ -409,6 +410,68 @@ fn connect_remote( remote_connection::connect_with_modal(&modal_workspace, connection_options, window, cx) } +/// A hash of the sidebar's entry list structure, used to detect flicker +/// (rapid A→B→A oscillations in `rebuild_contents`). +type ContentFingerprint = u64; + +fn compute_content_fingerprint(entries: &[ListEntry]) -> ContentFingerprint { + let mut hasher = std::hash::DefaultHasher::new(); + entries.len().hash(&mut hasher); + for entry in entries { + match entry { + ListEntry::ProjectHeader { + key, + is_active, + has_running_threads, + waiting_thread_count, + has_threads, + .. + } => { + 0u8.hash(&mut hasher); + key.hash(&mut hasher); + is_active.hash(&mut hasher); + has_running_threads.hash(&mut hasher); + waiting_thread_count.hash(&mut hasher); + has_threads.hash(&mut hasher); + } + ListEntry::Thread(thread) => { + 1u8.hash(&mut hasher); + thread.metadata.session_id.hash(&mut hasher); + thread.is_live.hash(&mut hasher); + thread.is_background.hash(&mut hasher); + std::mem::discriminant(&thread.status).hash(&mut hasher); + } + ListEntry::ViewMore { + key, + is_fully_expanded, + } => { + 2u8.hash(&mut hasher); + key.hash(&mut hasher); + is_fully_expanded.hash(&mut hasher); + } + ListEntry::DraftThread { key, workspace, .. } => { + 3u8.hash(&mut hasher); + key.hash(&mut hasher); + workspace + .as_ref() + .map(|ws| ws.entity_id()) + .hash(&mut hasher); + } + } + } + hasher.finish() +} + +const FLICKER_HISTORY_LEN: usize = 5; + +struct RebuildRecord { + fingerprint: ContentFingerprint, + caller: &'static std::panic::Location<'static>, + label: &'static str, + timestamp: std::time::Instant, + entry_count: usize, +} + /// The sidebar re-derives its entire entry list from scratch on every /// change via `update_entries` → `rebuild_contents`. Avoid adding /// incremental or inter-event coordination state — if something can @@ -446,6 +509,7 @@ pub struct Sidebar { project_header_menu_ix: Option, _subscriptions: Vec, _draft_observation: Option, + rebuild_history: VecDeque, } impl Sidebar { @@ -471,14 +535,14 @@ impl Sidebar { |this, _multi_workspace, event: &MultiWorkspaceEvent, window, cx| match event { MultiWorkspaceEvent::ActiveWorkspaceChanged => { this.observe_draft_editor(cx); - this.update_entries(cx); + this.update_entries("ActiveWorkspaceChanged", cx); } MultiWorkspaceEvent::WorkspaceAdded(workspace) => { this.subscribe_to_workspace(workspace, window, cx); - this.update_entries(cx); + this.update_entries("WorkspaceAdded", cx); } MultiWorkspaceEvent::WorkspaceRemoved(_) => { - this.update_entries(cx); + this.update_entries("WorkspaceRemoved", cx); } }, ) @@ -490,7 +554,7 @@ impl Sidebar { if !query.is_empty() { this.selection.take(); } - this.update_entries(cx); + this.update_entries("filter_editor::BufferEdited", cx); if !query.is_empty() { this.select_first_entry(); } @@ -499,7 +563,7 @@ impl Sidebar { .detach(); cx.observe(&ThreadMetadataStore::global(cx), |this, _store, cx| { - this.update_entries(cx); + this.update_entries("ThreadMetadataStore::changed", cx); }) .detach(); @@ -508,7 +572,7 @@ impl Sidebar { for workspace in &workspaces { this.subscribe_to_workspace(workspace, window, cx); } - this.update_entries(cx); + this.update_entries("Sidebar::new defer_in", cx); }); Self { @@ -534,6 +598,7 @@ impl Sidebar { project_header_menu_ix: None, _subscriptions: Vec::new(), _draft_observation: None, + rebuild_history: VecDeque::with_capacity(FLICKER_HISTORY_LEN), } } @@ -565,7 +630,7 @@ impl Sidebar { ProjectEvent::WorktreeAdded(_) | ProjectEvent::WorktreeRemoved(_) | ProjectEvent::WorktreeOrderChanged => { - this.update_entries(cx); + this.update_entries("ProjectEvent::WorktreeChanged", cx); } _ => {} }, @@ -585,7 +650,7 @@ impl Sidebar { _, ) ) { - this.update_entries(cx); + this.update_entries("GitWorktreeListChanged", cx); } }, ) @@ -637,14 +702,14 @@ impl Sidebar { } } this.observe_draft_editor(cx); - this.update_entries(cx); + this.update_entries("AgentPanelEvent::ThreadChanged", cx); } AgentPanelEvent::ThreadFocused | AgentPanelEvent::BackgroundThreadChanged => { - this.update_entries(cx); + this.update_entries("AgentPanelEvent::ThreadFocused", cx); } AgentPanelEvent::MessageSentOrQueued { session_id } => { this.record_thread_message_sent(session_id); - this.update_entries(cx); + this.update_entries("AgentPanelEvent::MessageSentOrQueued", cx); } }, ) @@ -790,7 +855,12 @@ impl Sidebar { /// - If you have no threads, and two workspaces for the worktree and the main workspace, make sure at least one is shown /// - Should always show every thread, associated with each workspace in the multiworkspace /// - After every build_contents, our "active" state should exactly match the current workspace's, current agent panel's current thread. - fn rebuild_contents(&mut self, cx: &App) { + fn rebuild_contents( + &mut self, + caller: &'static std::panic::Location<'static>, + label: &'static str, + cx: &App, + ) { let Some(multi_workspace) = self.multi_workspace.upgrade() else { return; }; @@ -1308,10 +1378,117 @@ impl Sidebar { project_header_indices, has_open_projects, }; + + let fingerprint = compute_content_fingerprint(&self.contents.entries); + + // Only record rebuilds that actually changed the list state. + // No-op rebuilds would fill the history window and push real + // state transitions out before we can detect oscillation. + let dominated_by_last = self + .rebuild_history + .back() + .is_some_and(|last| last.fingerprint == fingerprint); + if !dominated_by_last { + if self.rebuild_history.len() >= FLICKER_HISTORY_LEN { + self.rebuild_history.pop_front(); + } + self.rebuild_history.push_back(RebuildRecord { + fingerprint, + caller, + label, + timestamp: std::time::Instant::now(), + entry_count: self.contents.entries.len(), + }); + } + + // Detect flicker: if any earlier rebuild within the history window + // produced the same fingerprint as the current one (with at least one + // different fingerprint in between), something is oscillating. + if self.rebuild_history.len() >= 2 { + let current = self.rebuild_history.back().expect("just pushed"); + for (index, earlier) in self.rebuild_history.iter().enumerate() { + if index == self.rebuild_history.len() - 1 { + break; + } + if earlier.fingerprint != current.fingerprint { + continue; + } + // Same fingerprint found earlier — verify there's at least one + // *different* fingerprint in between so we don't fire on + // harmless no-op rebuilds (A→A→A). + let has_different_between = self + .rebuild_history + .iter() + .skip(index + 1) + .take(self.rebuild_history.len() - 1 - (index + 1)) + .any(|r| r.fingerprint != current.fingerprint); + if !has_different_between { + continue; + } + + // Assign a letter to each unique fingerprint for readability. + let mut fingerprint_labels: Vec = Vec::new(); + for record in &self.rebuild_history { + if !fingerprint_labels.contains(&record.fingerprint) { + fingerprint_labels.push(record.fingerprint); + } + } + let letter_for = |fp: ContentFingerprint| -> char { + let pos = fingerprint_labels + .iter() + .position(|&f| f == fp) + .unwrap_or(0); + (b'A' + pos as u8) as char + }; + + // Build the match pair description. + let matching_fp = current.fingerprint; + let matching_indices: Vec = self + .rebuild_history + .iter() + .enumerate() + .filter(|(_, r)| r.fingerprint == matching_fp) + .map(|(i, _)| i) + .collect(); + + use std::fmt::Write; + let mut message = format!( + "Sidebar flicker detected: state '{}' repeats at indices {:?}\n\ + \nFull rebuild history ({} entries, {} distinct states):", + letter_for(matching_fp), + matching_indices, + self.rebuild_history.len(), + fingerprint_labels.len(), + ); + let mut prev_ts: Option = None; + for (i, record) in self.rebuild_history.iter().enumerate() { + let delta = prev_ts + .map(|prev| record.timestamp.duration_since(prev)) + .unwrap_or_default(); + let letter = letter_for(record.fingerprint); + let marker = if record.fingerprint == matching_fp { + " <--" + } else { + "" + }; + write!( + &mut message, + "\n [{}] state={} entries={:<3} delta={:<10.1?} {} ({}){}", + i, letter, record.entry_count, delta, record.label, record.caller, marker, + ) + .ok(); + prev_ts = Some(record.timestamp); + } + util::debug_panic!("{message}"); + break; + } + } } /// Rebuilds the sidebar's visible entries from already-cached state. - fn update_entries(&mut self, cx: &mut Context) { + #[track_caller] + fn update_entries(&mut self, label: &'static str, cx: &mut Context) { + let caller = std::panic::Location::caller(); let Some(multi_workspace) = self.multi_workspace.upgrade() else { return; }; @@ -1322,7 +1499,7 @@ impl Sidebar { let had_notifications = self.has_notifications(cx); let scroll_position = self.list_state.logical_scroll_top(); - self.rebuild_contents(cx); + self.rebuild_contents(caller, label, cx); self.list_state.reset(self.contents.entries.len()); self.list_state.scroll_to(scroll_position); @@ -1601,7 +1778,7 @@ impl Sidebar { this.selection = None; this.expanded_groups.remove(&key_for_collapse); this.serialize(cx); - this.update_entries(cx); + this.update_entries("render_project_header::collapse", cx); } })), ) @@ -1895,7 +2072,7 @@ impl Sidebar { self.collapsed_groups.insert(project_group_key.clone()); } self.serialize(cx); - self.update_entries(cx); + self.update_entries("toggle_collapse", cx); } fn dispatch_context(&self, window: &Window, cx: &Context) -> KeyContext { @@ -1934,7 +2111,7 @@ impl Sidebar { fn cancel(&mut self, _: &Cancel, window: &mut Window, cx: &mut Context) { if self.reset_filter_editor_text(window, cx) { - self.update_entries(cx); + self.update_entries("cancel", cx); } else { self.selection = None; self.filter_editor.focus_handle(cx).focus(window, cx); @@ -2210,7 +2387,7 @@ impl Sidebar { Self::load_agent_thread_in_workspace(workspace, metadata, true, window, cx); - self.update_entries(cx); + self.update_entries("activate_thread_locally", cx); } fn activate_thread_in_other_window( @@ -2247,7 +2424,7 @@ impl Sidebar { workspace: workspace_for_entry.clone(), }); sidebar.record_thread_access(&target_session_id); - sidebar.update_entries(cx); + sidebar.update_entries("activate_thread_in_other_window", cx); }); } } @@ -2539,7 +2716,7 @@ impl Sidebar { Some(ListEntry::ProjectHeader { key, .. }) => { if self.collapsed_groups.contains(key) { self.collapsed_groups.remove(key); - self.update_entries(cx); + self.update_entries("expand_selected_entry", cx); } else if ix + 1 < self.contents.entries.len() { self.selection = Some(ix + 1); self.list_state.scroll_to_reveal_item(ix + 1); @@ -2562,7 +2739,7 @@ impl Sidebar { Some(ListEntry::ProjectHeader { key, .. }) => { if !self.collapsed_groups.contains(key) { self.collapsed_groups.insert(key.clone()); - self.update_entries(cx); + self.update_entries("collapse_selected_entry", cx); } } Some( @@ -2573,7 +2750,7 @@ impl Sidebar { { self.selection = Some(i); self.collapsed_groups.insert(key.clone()); - self.update_entries(cx); + self.update_entries("collapse_selected_entry", cx); break; } } @@ -2613,7 +2790,7 @@ impl Sidebar { self.selection = Some(header_ix); self.collapsed_groups.insert(key.clone()); } - self.update_entries(cx); + self.update_entries("toggle_selected_fold", cx); } } } @@ -2629,7 +2806,7 @@ impl Sidebar { self.collapsed_groups.insert(key.clone()); } } - self.update_entries(cx); + self.update_entries("fold_all", cx); } fn unfold_all( @@ -2639,7 +2816,7 @@ impl Sidebar { cx: &mut Context, ) { self.collapsed_groups.clear(); - self.update_entries(cx); + self.update_entries("unfold_all", cx); } fn stop_thread(&mut self, session_id: &acp::SessionId, cx: &mut Context) { @@ -3270,7 +3447,7 @@ impl Sidebar { session_id: metadata.session_id.clone(), workspace: workspace.clone(), }); - this.update_entries(cx); + this.update_entries("thread_switcher::Preview", cx); Self::load_agent_thread_in_workspace(workspace, metadata, false, window, cx); let focus = thread_switcher.focus_handle(cx); window.focus(&focus, cx); @@ -3290,7 +3467,7 @@ impl Sidebar { session_id: metadata.session_id.clone(), workspace: workspace.clone(), }); - this.update_entries(cx); + this.update_entries("thread_switcher::Confirmed", cx); Self::load_agent_thread_in_workspace(workspace, metadata, false, window, cx); this.dismiss_thread_switcher(cx); workspace.update(cx, |workspace, cx| { @@ -3312,7 +3489,7 @@ impl Sidebar { workspace: original_ws.clone(), }); } - this.update_entries(cx); + this.update_entries("thread_switcher::Dismissed", cx); if let Some(original_ws) = &original_workspace { Self::load_agent_thread_in_workspace( original_ws, @@ -3364,7 +3541,7 @@ impl Sidebar { session_id: metadata.session_id.clone(), workspace: workspace.clone(), }); - self.update_entries(cx); + self.update_entries("thread_switcher::initial_preview", cx); Self::load_agent_thread_in_workspace(&workspace, &metadata, false, window, cx); } @@ -3846,7 +4023,7 @@ impl Sidebar { self.expanded_groups .insert(project_group_key.clone(), current + 1); self.serialize(cx); - self.update_entries(cx); + self.update_entries("expand_thread_group", cx); } fn reset_thread_group_expansion( @@ -3856,7 +4033,7 @@ impl Sidebar { ) { self.expanded_groups.remove(project_group_key); self.serialize(cx); - self.update_entries(cx); + self.update_entries("reset_thread_group_expansion", cx); } fn collapse_thread_group( @@ -3875,7 +4052,7 @@ impl Sidebar { None => return, } self.serialize(cx); - self.update_entries(cx); + self.update_entries("collapse_thread_group", cx); } fn on_show_more_threads( @@ -4148,7 +4325,7 @@ impl Sidebar { .tooltip(Tooltip::text("Clear Search")) .on_click(cx.listener(|this, _, window, cx| { this.reset_filter_editor_text(window, cx); - this.update_entries(cx); + this.update_entries("clear_filter", cx); })), ) }), diff --git a/crates/sidebar/src/sidebar_tests.rs b/crates/sidebar/src/sidebar_tests.rs index 6a3da0a1d07ae66b4012b87e4533ed163115f4c3..147745b711ad7338b67510b0190c60b67f8d4140 100644 --- a/crates/sidebar/src/sidebar_tests.rs +++ b/crates/sidebar/src/sidebar_tests.rs @@ -693,7 +693,7 @@ async fn test_view_more_batched_expansion(cx: &mut TestAppContext) { .unwrap_or(0); s.expanded_groups .insert(project_group_key.clone(), current + 1); - s.update_entries(cx); + s.update_entries("test", cx); }); cx.run_until_parked(); @@ -711,7 +711,7 @@ async fn test_view_more_batched_expansion(cx: &mut TestAppContext) { .unwrap_or(0); s.expanded_groups .insert(project_group_key.clone(), current + 1); - s.update_entries(cx); + s.update_entries("test", cx); }); cx.run_until_parked(); @@ -724,7 +724,7 @@ async fn test_view_more_batched_expansion(cx: &mut TestAppContext) { // Click collapse - should go back to showing 5 threads sidebar.update_in(cx, |s, _window, cx| { s.expanded_groups.remove(&project_group_key); - s.update_entries(cx); + s.update_entries("test", cx); }); cx.run_until_parked(); @@ -5068,7 +5068,7 @@ async fn test_switch_to_workspace_with_archived_thread_shows_draft(cx: &mut Test cx.run_until_parked(); sidebar.update_in(cx, |sidebar, _window, cx| { - sidebar.update_entries(cx); + sidebar.update_entries("test", cx); }); cx.run_until_parked(); @@ -5650,7 +5650,7 @@ async fn test_linked_worktree_workspace_reachable_and_dismissable(cx: &mut TestA cx.run_until_parked(); sidebar.update_in(cx, |sidebar, _window, cx| { - sidebar.update_entries(cx); + sidebar.update_entries("test", cx); }); cx.run_until_parked(); @@ -6207,7 +6207,7 @@ async fn test_linked_worktree_workspace_reachable_after_adding_unrelated_project for group_key in group_keys { sidebar.expanded_groups.insert(group_key, 10_000); } - sidebar.update_entries(cx); + sidebar.update_entries("test", cx); }); cx.run_until_parked(); @@ -6630,7 +6630,7 @@ mod property_test { for group_key in group_keys { sidebar.expanded_groups.insert(group_key, 10_000); } - sidebar.update_entries(cx); + sidebar.update_entries("test", cx); }); }