-
Notifications
You must be signed in to change notification settings - Fork 0
Add agent tabs #8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extracted tab-related codes here to reduce chance of conflicts when syncing with upstream
| /// Removes the currently selected thread. | ||
| RemoveSelectedThread, | ||
| /// Closes the currently active thread tab. | ||
| CloseActiveThreadTab, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
|
||
| // Methods to manage tabs in AgentPanel | ||
| impl AgentPanel { | ||
| fn active_tab(&self) -> &AgentPanelTab { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential panic risk in active_tab() method. The unwrap_or_else(|| &self.tabs[0]) could panic if self.tabs is empty. Consider handling this case more explicitly.
| let removed_id = id; | ||
| self.tabs.remove(removed_id); | ||
| let new_id = if self.active_tab_id == removed_id { | ||
| removed_id.min(self.tabs.len() - 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential panic risk in remove_tab_by_id() method at line 2440. The expression removed_id.min(self.tabs.len() - 1) could panic if self.tabs becomes empty after removal. Consider checking if self.tabs is empty before calculating new_id.
| fn active_tab(&self) -> &AgentPanelTab { | ||
| self.tabs | ||
| .get(self.active_tab_id) | ||
| .unwrap_or_else(|| &self.tabs[0]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This indexing could panic if tabs is empty. Consider adding a guard or using .get() with proper error handling.
|
|
||
| pub fn session_id(&self, cx: &App) -> Option<acp::SessionId> { | ||
| if let Some(thread) = self.thread() { | ||
| Some(thread.read(cx).session_id().clone()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using .map() instead of .clone() inside the closure since session_id() already returns a reference that can be cloned if needed.
| let removed_id = id; | ||
| self.tabs.remove(removed_id); | ||
| let new_id = if self.active_tab_id == removed_id { | ||
| removed_id.min(self.tabs.len() - 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding a guard to ensure tabs.len() - 1 doesn't underflow when tabs is empty.
| } | ||
|
|
||
| fn set_active_tab_by_id(&mut self, new_id: TabId, window: &mut Window, cx: &mut Context<Self>) { | ||
| // TODO: need to check the total items in the list, if it is equal to 1, we should overlay it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO comment should be addressed before merging - consider implementing the logic to overlay when there is only one tab.

Integrate changes from zed-industries#42387 to implement agent tabs.
ux-zed-multitabs.mov.mp4