Skip to content

Commit 67dc631

Browse files
authored
Consistent Tab IDs & Global Tag Targeting (#892)
Introduces stable per-tab IDs and a global `--tab <id>` flag for scoping individual commands to a specific tab. Breaking change: response payloads for `tab_list`, `tab_new`, `tab_switch`, `tab_close`, and `window_new` now use `tabId` instead of `index`. `tab_close` returns `{tabId, closed: true}` instead of `{closed, activeIndex}`. `agent-browser tab <unknown>` now errors instead of silently listing tabs. Follow-up PR to land immediately after this fixes a compile error on the provider direct-page path, clears per-tab daemon state around scoped switches, and implements active-tab restoration so `--tab N` is non-intrusive as intended.
1 parent c691b26 commit 67dc631

7 files changed

Lines changed: 618 additions & 45 deletions

File tree

cli/src/commands.rs

Lines changed: 64 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1010,28 +1010,35 @@ fn parse_command_inner(args: &[String], flags: &Flags) -> Result<Value, ParseErr
10101010
}
10111011

10121012
// === Tabs ===
1013-
"tab" => match rest.first().copied() {
1014-
Some("new") => {
1015-
let mut cmd = json!({ "id": id, "action": "tab_new" });
1016-
if let Some(url) = rest.get(1) {
1017-
cmd["url"] = json!(url);
1013+
"tab" => {
1014+
const VALID: &[&str] = &["list", "new", "close", "<id>"];
1015+
match rest.first().copied() {
1016+
Some("new") => {
1017+
let mut cmd = json!({ "id": id, "action": "tab_new" });
1018+
if let Some(url) = rest.get(1) {
1019+
cmd["url"] = json!(url);
1020+
}
1021+
Ok(cmd)
10181022
}
1019-
Ok(cmd)
1020-
}
1021-
Some("list") => Ok(json!({ "id": id, "action": "tab_list" })),
1022-
Some("close") => {
1023-
let mut cmd = json!({ "id": id, "action": "tab_close" });
1024-
if let Some(index) = rest.get(1).and_then(|s| s.parse::<i32>().ok()) {
1025-
cmd["index"] = json!(index);
1023+
Some("list") => Ok(json!({ "id": id, "action": "tab_list" })),
1024+
Some("close") => {
1025+
let mut cmd = json!({ "id": id, "action": "tab_close" });
1026+
if let Some(tab_id) = rest.get(1).and_then(|s| s.parse::<i32>().ok()) {
1027+
cmd["tabId"] = json!(tab_id);
1028+
}
1029+
Ok(cmd)
10261030
}
1027-
Ok(cmd)
1028-
}
1029-
Some(n) if n.parse::<i32>().is_ok() => {
1030-
let index = n.parse::<i32>().expect("already checked parse succeeds");
1031-
Ok(json!({ "id": id, "action": "tab_switch", "index": index }))
1031+
Some(n) if n.parse::<i32>().is_ok() => {
1032+
let tab_id = n.parse::<i32>().expect("already checked parse succeeds");
1033+
Ok(json!({ "id": id, "action": "tab_switch", "tabId": tab_id }))
1034+
}
1035+
Some(sub) => Err(ParseError::UnknownSubcommand {
1036+
subcommand: sub.to_string(),
1037+
valid_options: VALID,
1038+
}),
1039+
None => Ok(json!({ "id": id, "action": "tab_list" })),
10321040
}
1033-
_ => Ok(json!({ "id": id, "action": "tab_list" })),
1034-
},
1041+
}
10351042

10361043
// === Window ===
10371044
"window" => {
@@ -2335,6 +2342,7 @@ mod tests {
23352342
fn default_flags() -> Flags {
23362343
Flags {
23372344
session: "test".to_string(),
2345+
tab: None,
23382346
json: false,
23392347
headed: false,
23402348
debug: false,
@@ -2847,7 +2855,7 @@ mod tests {
28472855
fn test_tab_switch() {
28482856
let cmd = parse_command(&args("tab 2"), &default_flags()).unwrap();
28492857
assert_eq!(cmd["action"], "tab_switch");
2850-
assert_eq!(cmd["index"], 2);
2858+
assert_eq!(cmd["tabId"], 2);
28512859
}
28522860

28532861
#[test]
@@ -2856,6 +2864,42 @@ mod tests {
28562864
assert_eq!(cmd["action"], "tab_close");
28572865
}
28582866

2867+
#[test]
2868+
fn test_tab_close_with_id() {
2869+
let cmd = parse_command(&args("tab close 2"), &default_flags()).unwrap();
2870+
assert_eq!(cmd["action"], "tab_close");
2871+
assert_eq!(cmd["tabId"], 2);
2872+
}
2873+
2874+
#[test]
2875+
fn test_tab_switch_sends_tab_id() {
2876+
let cmd = parse_command(&args("tab 2"), &default_flags()).unwrap();
2877+
assert_eq!(cmd["tabId"], 2);
2878+
assert!(cmd.get("index").is_none());
2879+
}
2880+
2881+
#[test]
2882+
fn test_tab_close_sends_tab_id() {
2883+
let cmd = parse_command(&args("tab close 3"), &default_flags()).unwrap();
2884+
assert_eq!(cmd["tabId"], 3);
2885+
assert!(cmd.get("index").is_none());
2886+
}
2887+
2888+
#[test]
2889+
fn test_tab_no_args_defaults_to_list() {
2890+
let cmd = parse_command(&args("tab"), &default_flags()).unwrap();
2891+
assert_eq!(cmd["action"], "tab_list");
2892+
}
2893+
2894+
#[test]
2895+
fn test_tab_unknown_subcommand_errors() {
2896+
let result = parse_command(&args("tab select 3"), &default_flags());
2897+
assert!(
2898+
result.is_err(),
2899+
"tab select should error, not silently fall through to tab_list"
2900+
);
2901+
}
2902+
28592903
// === Network ===
28602904

28612905
#[test]

cli/src/flags.rs

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,7 @@ pub struct Config {
5757
pub json: Option<bool>,
5858
pub debug: Option<bool>,
5959
pub session: Option<String>,
60+
pub tab: Option<u32>,
6061
pub session_name: Option<String>,
6162
pub executable_path: Option<String>,
6263
pub extensions: Option<Vec<String>>,
@@ -98,6 +99,7 @@ impl Config {
9899
json: other.json.or(self.json),
99100
debug: other.debug.or(self.debug),
100101
session: other.session.or(self.session),
102+
tab: other.tab.or(self.tab),
101103
session_name: other.session_name.or(self.session_name),
102104
executable_path: other.executable_path.or(self.executable_path),
103105
extensions: match (self.extensions, other.extensions) {
@@ -196,6 +198,7 @@ fn parse_bool_arg(args: &[String], i: usize) -> (bool, bool) {
196198
fn extract_config_path(args: &[String]) -> Option<Option<String>> {
197199
const FLAGS_WITH_VALUE: &[&str] = &[
198200
"--session",
201+
"--tab",
199202
"--headers",
200203
"--executable-path",
201204
"--cdp",
@@ -273,6 +276,7 @@ pub struct Flags {
273276
pub headed: bool,
274277
pub debug: bool,
275278
pub session: String,
279+
pub tab: Option<u32>,
276280
pub headers: Option<String>,
277281
pub executable_path: Option<String>,
278282
pub cdp: Option<String>,
@@ -355,6 +359,7 @@ pub fn parse_flags(args: &[String]) -> Flags {
355359
.ok()
356360
.or(config.session)
357361
.unwrap_or_else(|| "default".to_string()),
362+
tab: config.tab,
358363
headers: config.headers,
359364
executable_path: env::var("AGENT_BROWSER_EXECUTABLE_PATH")
360365
.ok()
@@ -492,6 +497,12 @@ pub fn parse_flags(args: &[String]) -> Flags {
492497
i += 1;
493498
}
494499
}
500+
"--tab" => {
501+
if let Some(s) = args.get(i + 1) {
502+
flags.tab = s.parse::<u32>().ok();
503+
i += 1;
504+
}
505+
}
495506
"--idle-timeout" => {
496507
if let Some(s) = args.get(i + 1) {
497508
match parse_idle_timeout(s) {
@@ -775,6 +786,7 @@ pub fn clean_args(args: &[String]) -> Vec<String> {
775786
// Global flags that always take a value (need to skip the next arg too)
776787
const GLOBAL_FLAGS_WITH_VALUE: &[&str] = &[
777788
"--session",
789+
"--tab",
778790
"--headers",
779791
"--executable-path",
780792
"--cdp",
@@ -1441,4 +1453,25 @@ mod tests {
14411453
let clean = clean_args(&input);
14421454
assert_eq!(clean, vec!["open", "example.com"]);
14431455
}
1456+
1457+
// === Tab flag tests ===
1458+
1459+
#[test]
1460+
fn test_parse_tab_flag() {
1461+
let flags = parse_flags(&args("--tab 4 snapshot"));
1462+
assert_eq!(flags.tab, Some(4));
1463+
}
1464+
1465+
#[test]
1466+
fn test_clean_args_removes_tab_flag() {
1467+
let cleaned = clean_args(&args("--tab 4 snapshot"));
1468+
assert_eq!(cleaned, vec!["snapshot"]);
1469+
}
1470+
1471+
#[test]
1472+
fn test_parse_tab_config() {
1473+
let json = r#"{"tab": 4}"#;
1474+
let config: Config = serde_json::from_str(json).unwrap();
1475+
assert_eq!(config.tab, Some(4));
1476+
}
14441477
}

cli/src/main.rs

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -738,6 +738,12 @@ fn main() {
738738
}
739739
};
740740

741+
if let Some(tab_id) = flags.tab {
742+
if cmd.get("tabId").is_none() {
743+
cmd["tabId"] = json!(tab_id);
744+
}
745+
}
746+
741747
// Handle --password-stdin for auth save
742748
if cmd.get("action").and_then(|v| v.as_str()) == Some("auth_save") {
743749
if cmd.get("password").is_some() {

cli/src/native/actions.rs

Lines changed: 27 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -662,7 +662,9 @@ impl DaemonState {
662662
.await;
663663
}
664664

665+
let tab_id = mgr.assign_tab_id();
665666
mgr.add_page(super::browser::PageInfo {
667+
tab_id,
666668
target_id: te.target_info.target_id.clone(),
667669
session_id: attach.session_id,
668670
url: te.target_info.url.clone(),
@@ -1273,6 +1275,20 @@ pub async fn execute_command(cmd: &Value, state: &mut DaemonState) -> Value {
12731275
);
12741276
}
12751277

1278+
// Pre-dispatch: if tabId is set on a non-tab command, switch to that tab first
1279+
if !matches!(
1280+
action,
1281+
"tab_list" | "tab_new" | "tab_switch" | "tab_close" | "launch" | "close"
1282+
) {
1283+
if let Some(tab_id) = cmd.get("tabId").and_then(|v| v.as_u64()) {
1284+
if let Some(ref mut mgr) = state.browser {
1285+
if let Err(e) = mgr.tab_switch_by_id(tab_id as u32).await {
1286+
return error_response(&id, &e);
1287+
}
1288+
}
1289+
}
1290+
}
1291+
12761292
let result = match action {
12771293
"launch" => handle_launch(cmd, state).await,
12781294
"navigate" => handle_navigate(cmd, state).await,
@@ -3647,14 +3663,14 @@ async fn handle_tab_new(cmd: &Value, state: &mut DaemonState) -> Result<Value, S
36473663

36483664
async fn handle_tab_switch(cmd: &Value, state: &mut DaemonState) -> Result<Value, String> {
36493665
let mgr = state.browser.as_mut().ok_or("Browser not launched")?;
3650-
let index = cmd
3651-
.get("index")
3666+
let tab_id = cmd
3667+
.get("tabId")
36523668
.and_then(|v| v.as_u64())
3653-
.ok_or("Missing 'index' parameter")? as usize;
3669+
.ok_or("Missing 'tabId' parameter")? as u32;
36543670
state.ref_map.clear();
36553671
state.iframe_sessions.clear();
36563672
state.active_frame_id = None;
3657-
let result = mgr.tab_switch(index).await?;
3673+
let result = mgr.tab_switch_by_id(tab_id).await?;
36583674

36593675
if let Some(ref server) = state.stream_server {
36603676
if let Ok(dims) = mgr
@@ -3679,14 +3695,11 @@ async fn handle_tab_switch(cmd: &Value, state: &mut DaemonState) -> Result<Value
36793695

36803696
async fn handle_tab_close(cmd: &Value, state: &mut DaemonState) -> Result<Value, String> {
36813697
let mgr = state.browser.as_mut().ok_or("Browser not launched")?;
3682-
let index = cmd
3683-
.get("index")
3684-
.and_then(|v| v.as_u64())
3685-
.map(|i| i as usize);
3698+
let tab_id = cmd.get("tabId").and_then(|v| v.as_u64()).map(|i| i as u32);
36863699
state.ref_map.clear();
36873700
state.iframe_sessions.clear();
36883701
state.active_frame_id = None;
3689-
mgr.tab_close(index).await
3702+
mgr.tab_close_by_id(tab_id).await
36903703
}
36913704

36923705
async fn handle_viewport(cmd: &Value, state: &mut DaemonState) -> Result<Value, String> {
@@ -4065,7 +4078,9 @@ async fn handle_recording_start(cmd: &Value, state: &mut DaemonState) -> Result<
40654078
}
40664079

40674080
// Add page and switch to it
4081+
let tab_id = mgr.assign_tab_id();
40684082
mgr.add_page(super::browser::PageInfo {
4083+
tab_id,
40694084
target_id: create_result.target_id,
40704085
session_id: new_session_id.clone(),
40714086
url: nav_url.clone(),
@@ -5976,7 +5991,9 @@ async fn handle_window_new(cmd: &Value, state: &mut DaemonState) -> Result<Value
59765991
)
59775992
.await?;
59785993

5994+
let tab_id = mgr.assign_tab_id();
59795995
mgr.add_page(super::browser::PageInfo {
5996+
tab_id,
59805997
target_id: create_result.target_id,
59815998
session_id: attach.session_id,
59825999
url: "about:blank".to_string(),
@@ -6004,7 +6021,7 @@ async fn handle_window_new(cmd: &Value, state: &mut DaemonState) -> Result<Value
60046021
let total = mgr.page_count();
60056022
state.ref_map.clear();
60066023

6007-
Ok(json!({ "index": total - 1, "total": total }))
6024+
Ok(json!({ "tabId": tab_id, "total": total }))
60086025
}
60096026

60106027
async fn handle_diff_screenshot(cmd: &Value, state: &DaemonState) -> Result<Value, String> {

0 commit comments

Comments
 (0)