Skip to content

fix: stabilize tab IDs to prevent index drift when tabs are closed externally #1230

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.

Sign up for GitHub

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

Open
yangyaofei wants to merge 1 commit into
base: main
Choose a base branch
from
+65 −29
Open

fix: stabilize tab IDs to prevent index drift when tabs are closed externally #1230

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
89 changes: 62 additions & 27 deletions cli/src/native/browser.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,10 @@ pub struct BrowserManager {
pub ignore_https_errors: bool,
/// Origins visited during this session, used by save_state to collect cross-origin localStorage.
visited_origins: HashSet<String>,
/// Stable tab IDs: maps target_id -> stable tab id (1-based, never reused)
tab_ids: HashMap<String, usize>,
/// Next tab id to assign
next_tab_id: usize,
}

const LIGHTPANDA_CDP_CONNECT_TIMEOUT: Duration = Duration::from_secs(5);
Expand Down Expand Up @@ -277,6 +281,8 @@ impl BrowserManager {
download_path: download_path.clone(),
ignore_https_errors,
visited_origins: HashSet::new(),
tab_ids: HashMap::new(),
next_tab_id: 1,
};
manager.discover_and_attach_targets().await?;
manager
Expand Down Expand Up @@ -365,11 +371,17 @@ impl BrowserManager {
download_path: None,
ignore_https_errors: false,
visited_origins: HashSet::new(),
tab_ids: HashMap::new(),
next_tab_id: 1,
};

if direct_page {
let tab_id = manager.next_tab_id;
manager.next_tab_id += 1;
let target_id = "provider-page".to_string();
manager.tab_ids.insert(target_id.clone(), tab_id);
manager.pages.push(PageInfo {
target_id: "provider-page".to_string(),
target_id,
session_id: String::new(),
url: String::new(),
title: String::new(),
Expand Down Expand Up @@ -433,8 +445,12 @@ impl BrowserManager {
)
.await?;

let tab_id = self.next_tab_id;
self.next_tab_id += 1;
let target_id = result.target_id.clone();
self.tab_ids.insert(target_id.clone(), tab_id);
self.pages.push(PageInfo {
target_id: result.target_id,
target_id,
session_id: attach_result.session_id.clone(),
url: "about:blank".to_string(),
title: String::new(),
Expand All @@ -456,6 +472,9 @@ impl BrowserManager {
)
.await?;

let tab_id = self.next_tab_id;
self.next_tab_id += 1;
self.tab_ids.insert(target.target_id.clone(), tab_id);
self.pages.push(PageInfo {
target_id: target.target_id.clone(),
session_id: attach_result.session_id.clone(),
Expand Down Expand Up @@ -484,9 +503,6 @@ impl BrowserManager {
self.client
.send_command_no_params("Runtime.enable", Some(session_id))
.await?;
// Resume the target if it is paused waiting for the debugger.
// This is needed for real browser sessions (Chrome 144+) where targets
// are paused after attach until explicitly resumed. No-op otherwise.
let _ = self
.client
.send_command_no_params("Runtime.runIfWaitingForDebugger", Some(session_id))
Expand Down Expand Up @@ -835,8 +851,9 @@ impl BrowserManager {
.iter()
.enumerate()
.map(|(i, p)| {
let tab_id = self.tab_ids.get(&p.target_id).copied().unwrap_or(0);
json!({
"index": i,
"index": tab_id,
"title": p.title,
"url": p.url,
"type": p.target_type,
Expand Down Expand Up @@ -874,33 +891,44 @@ impl BrowserManager {

self.enable_domains(&attach.session_id).await?;

let tab_id = self.next_tab_id;
self.next_tab_id += 1;
let index = self.pages.len();
let target_id = result.target_id.clone();
self.tab_ids.insert(target_id.clone(), tab_id);
self.pages.push(PageInfo {
target_id: result.target_id,
target_id,
session_id: attach.session_id,
url: target_url.to_string(),
title: String::new(),
target_type: "page".to_string(),
});
self.active_page_index = index;

Ok(json!({ "index": index, "url": target_url }))
Ok(json!({ "index": tab_id, "url": target_url }))
}

pub async fn tab_switch(&mut self, index: usize) -> Result<Value, String> {
if index >= self.pages.len() {
return Err(format!(
"Tab index {} out of range (0-{})",
index,
self.pages.len().saturating_sub(1)
));
}
fn resolve_tab_index(&self, tab_id: usize) -> Option<usize> {
let target_id = self.tab_ids.iter().find(|(_, &id)| id == tab_id)?.0;
self.pages.iter().position(|p| p.target_id == *target_id)
}

pub fn active_tab_id(&self) -> usize {
self.pages
.get(self.active_page_index)
.and_then(|p| self.tab_ids.get(&p.target_id).copied())
.unwrap_or(0)
}

pub async fn tab_switch(&mut self, tab_id: usize) -> Result<Value, String> {
let index = self.resolve_tab_index(tab_id).ok_or_else(|| {
format!("Tab id {} not found", tab_id)
})?;

self.active_page_index = index;
let session_id = self.pages[index].session_id.clone();
self.enable_domains(&session_id).await?;

// Bring tab to front
let _ = self
.client
.send_command("Page.bringToFront", None, Some(&session_id))
Expand All @@ -914,27 +942,27 @@ impl BrowserManager {
page.title = title.clone();
}

Ok(json!({ "index": index, "url": url, "title": title }))
Ok(json!({ "index": tab_id, "url": url, "title": title }))
}

pub async fn tab_close(&mut self, index: Option<usize>) -> Result<Value, String> {
let target_index = index.unwrap_or(self.active_page_index);

if target_index >= self.pages.len() {
return Err(format!("Tab index {} out of range", target_index));
}
pub async fn tab_close(&mut self, tab_id: Option<usize>) -> Result<Value, String> {
let target_tab_id = tab_id.unwrap_or(self.active_tab_id());
let index = self.resolve_tab_index(target_tab_id).ok_or_else(|| {
format!("Tab id {} not found", target_tab_id)
})?;

if self.pages.len() <= 1 {
return Err("Cannot close the last tab".to_string());
}

let page = self.pages.remove(target_index);
let page = self.pages.remove(index);
self.tab_ids.remove(&page.target_id);
let _ = self
.client
.send_command_typed::<_, Value>(
"Target.closeTarget",
&CloseTargetParams {
target_id: page.target_id,
target_id: page.target_id.clone(),
},
None,
)
Expand All @@ -947,7 +975,8 @@ impl BrowserManager {
let session_id = self.pages[self.active_page_index].session_id.clone();
self.enable_domains(&session_id).await?;

Ok(json!({ "closed": target_index, "activeIndex": self.active_page_index }))
let active_tab_id = self.active_tab_id();
Ok(json!({ "closed": target_tab_id, "activeIndex": active_tab_id }))
}

// -----------------------------------------------------------------------
Expand Down Expand Up @@ -1189,6 +1218,9 @@ impl BrowserManager {
}

pub fn add_page(&mut self, page: PageInfo) {
let tab_id = self.next_tab_id;
self.next_tab_id += 1;
self.tab_ids.insert(page.target_id.clone(), tab_id);
let index = self.pages.len();
self.pages.push(page);
self.active_page_index = index;
Expand All @@ -1201,6 +1233,7 @@ impl BrowserManager {
pub fn remove_page_by_target_id(&mut self, target_id: &str) {
if let Some(pos) = self.pages.iter().position(|p| p.target_id == target_id) {
self.pages.remove(pos);
self.tab_ids.remove(target_id);
self.update_active_page_if_needed();
}
}
Expand Down Expand Up @@ -1370,6 +1403,8 @@ async fn initialize_lightpanda_manager(
download_path: None,
ignore_https_errors: false,
visited_origins: HashSet::new(),
tab_ids: HashMap::new(),
next_tab_id: 1,
};

match discover_and_attach_lightpanda_targets(&mut manager, deadline).await {
Expand Down
5 changes: 3 additions & 2 deletions cli/src/output.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -381,19 +381,20 @@ pub fn print_response_with_opts(resp: &Response, action: Option<&str>, opts: &Ou
}
// Tabs
if let Some(tabs) = data.get("tabs").and_then(|v| v.as_array()) {
for (i, tab) in tabs.iter().enumerate() {
for tab in tabs.iter() {
let title = tab
.get("title")
.and_then(|v| v.as_str())
.unwrap_or("Untitled");
let url = tab.get("url").and_then(|v| v.as_str()).unwrap_or("");
let active = tab.get("active").and_then(|v| v.as_bool()).unwrap_or(false);
let idx = tab.get("index").and_then(|v| v.as_u64()).unwrap_or(0);
let marker = if active {
color::cyan("→")
} else {
" ".to_string()
};
println!("{} [{}] {} - {}", marker, i, title, url);
println!("{} [{}] {} - {}", marker, idx, title, url);
}
return;
}
Expand Down