Fix three code quality issues found in telemetry review

Katie Geer created 1 week ago

Startup false-positive in Sidebar Toggled:
  open_sidebar was called from workspace state restoration on every
  startup when the sidebar had been left open, which fired the
  'Sidebar Toggled {action=open}' telemetry event even though no user
  action occurred. Extract apply_open_sidebar as the shared
  implementation, keep open_sidebar for user-triggered opens (fires
  event), and add restore_open_sidebar for the startup path (no event).

Wildcard arm in Agent Panel Side Changed:
  set_position used '_ => "right"' which silently mapped
  DockPosition::Bottom to "right". Made the match exhaustive with
  explicit Right | Bottom arms so a future DockPosition variant would
  cause a compile error rather than silent misreporting.

Duplicated thread_location logic:
  The NewThreadLocation -> &'static str mapping appeared three times
  verbatim (new_thread, new_thread_from_sidebar, menu builder closure),
  each with its own local 'use settings::...' import. Extracted into a
  thread_location_str(cx: &App) free function alongside the existing
  agent_panel_dock_position helper.

Change summary

crates/agent_ui/src/agent_panel.rs      | 31 ++++++++++----------------
crates/workspace/src/multi_workspace.rs | 10 ++++++++
crates/workspace/src/workspace.rs       |  2 
3 files changed, 23 insertions(+), 20 deletions(-)

Detailed changes

crates/agent_ui/src/agent_panel.rs

@@ -1304,29 +1304,19 @@ impl AgentPanel {
     }
 
     pub fn new_thread(&mut self, _action: &NewThread, window: &mut Window, cx: &mut Context<Self>) {
-        use settings::{NewThreadLocation, Settings};
-        let thread_location = match AgentSettings::get_global(cx).new_thread_location {
-            NewThreadLocation::LocalProject => "current_worktree",
-            NewThreadLocation::NewWorktree => "new_worktree",
-        };
         telemetry::event!(
             "New Thread Clicked",
             source = "agent_panel",
-            thread_location = thread_location
+            thread_location = thread_location_str(cx)
         );
         self.do_new_thread(window, cx);
     }
 
     pub fn new_thread_from_sidebar(&mut self, window: &mut Window, cx: &mut Context<Self>) {
-        use settings::{NewThreadLocation, Settings};
-        let thread_location = match AgentSettings::get_global(cx).new_thread_location {
-            NewThreadLocation::LocalProject => "current_worktree",
-            NewThreadLocation::NewWorktree => "new_worktree",
-        };
         telemetry::event!(
             "New Thread Clicked",
             source = "sidebar",
-            thread_location = thread_location
+            thread_location = thread_location_str(cx)
         );
         self.do_new_thread(window, cx);
     }
@@ -3344,6 +3334,14 @@ fn agent_panel_dock_position(cx: &App) -> DockPosition {
     AgentSettings::get_global(cx).dock.into()
 }
 
+fn thread_location_str(cx: &App) -> &'static str {
+    use settings::{NewThreadLocation, Settings};
+    match AgentSettings::get_global(cx).new_thread_location {
+        NewThreadLocation::LocalProject => "current_worktree",
+        NewThreadLocation::NewWorktree => "new_worktree",
+    }
+}
+
 pub enum AgentPanelEvent {
     ActiveViewChanged,
     ThreadFocused,
@@ -3374,7 +3372,7 @@ impl Panel for AgentPanel {
     fn set_position(&mut self, position: DockPosition, _: &mut Window, cx: &mut Context<Self>) {
         let side = match position {
             DockPosition::Left => "left",
-            _ => "right",
+            DockPosition::Right | DockPosition::Bottom => "right",
         };
         telemetry::event!("Agent Panel Side Changed", side = side);
         settings::update_settings_file(self.fs.clone(), cx, move |settings, _| {
@@ -3805,15 +3803,10 @@ impl AgentPanel {
             let agent_server_store = agent_server_store;
 
             Rc::new(move |window, cx| {
-                use settings::{NewThreadLocation, Settings};
-                let thread_location = match AgentSettings::get_global(cx).new_thread_location {
-                    NewThreadLocation::LocalProject => "current_worktree",
-                    NewThreadLocation::NewWorktree => "new_worktree",
-                };
                 telemetry::event!(
                     "New Thread Clicked",
                     source = "agent_panel",
-                    thread_location = thread_location
+                    thread_location = thread_location_str(cx)
                 );
 
                 let active_thread = active_thread.clone();

crates/workspace/src/multi_workspace.rs

@@ -473,6 +473,16 @@ impl MultiWorkspace {
             SidebarSide::Right => "right",
         };
         telemetry::event!("Sidebar Toggled", action = "open", side = side);
+        self.apply_open_sidebar(cx);
+    }
+
+    /// Restores the sidebar to open state from persisted session data without
+    /// firing a telemetry event, since this is not a user-initiated action.
+    pub(crate) fn restore_open_sidebar(&mut self, cx: &mut Context<Self>) {
+        self.apply_open_sidebar(cx);
+    }
+
+    fn apply_open_sidebar(&mut self, cx: &mut Context<Self>) {
         self.sidebar_open = true;
         if let ActiveWorkspace::Transient(workspace) = &self.active_workspace {
             let workspace = workspace.clone();

crates/workspace/src/workspace.rs

@@ -8808,7 +8808,7 @@ pub async fn restore_multiworkspace(
     if sidebar_open {
         window_handle
             .update(cx, |multi_workspace, _, cx| {
-                multi_workspace.open_sidebar(cx);
+                multi_workspace.restore_open_sidebar(cx);
             })
             .ok();
     }