feat(tabs): t<N> prefix for tab ids; --label for named tabs; drop --tab peek flag (#1250)
* fix(tabs): preserve refs across --tab peek and cover outer-tab-closed path Follow-up to #1249 so `--tab <id>` is actually useful for agents: - Save and restore the outer tab's `ref_map`, `iframe_sessions`, and `active_frame_id` across a scoped command instead of clearing them. `snapshot` → `--tab N <cmd>` → `click @e1` now keeps the outer tab's refs intact. Scoped commands still see a clean slate so outer refs can't resolve against the scoped tab's DOM. - Close the coverage gap the Vercel review bot flagged on #1249: the previous `e2e_tab_scoped_command_handles_outer_tab_closed` test used `tab_close`, which is in the scoped-dispatch exclusion list, so it never exercised the restore-skip branch it claimed to test. Renamed to `e2e_tab_close_with_tab_id_closes_active_tab` with an honest docstring, and added `e2e_tab_scoped_command_outer_tab_closed_mid_dispatch` that actually hits the branch via `window.opener.close()` on a script-opened intermediate tab. - Add `e2e_tab_scoped_command_isolates_refs_from_outer_tab` pinning that outer refs don't bleed into the scoped tab's DOM resolution. - Rewrite `e2e_tab_scoped_command_clears_state_on_switch` as `e2e_tab_scoped_command_preserves_outer_tab_state`, verifying the restored @e1 still clicks end-to-end. - Update the 52 `--help` entries for `--tab <id>` to describe peek / restore semantics instead of a vague "Target specific tab ID". - Update README, docs site, config schema, and the agent-facing skills reference with working examples (refs survive the peek) and a "when to use \`--tab <id>\` vs \`tab <id>\`" guide so agents pick the right flag for their workflow. * fix(tabs): use t<N> prefix for tab ids, add --label for named tabs Follow-on to the tab work in #1249 and the prior commit, redesigning the tab handle surface before release since nothing ships these features yet. ## Why Incrementing integer tab ids (`1`, `2`, `3`) look indistinguishable from positional indices in command output, LLM-generated scripts, and docs. In the common single-agent case where position and id coincide, readers have no visual cue for which mental model they're using. Positional indices silently shift when unrelated tabs open/close, so misreading a handle as an index is a correctness hazard. ## Changes **Tab ids are now `t1`, `t2`, `t3` (strings).** Bare integer `tabId` values are rejected with a teaching message rather than silently accepted. The `t` prefix matches the `@e1` element-ref convention and makes ids unmistakably non-positional at a glance. **Labels.** Tabs can be created with a user-assigned label (e.g. `docs`, `app`) via `tab new --label <name> [url]`. Labels are interchangeable with `t<N>` ids everywhere a tab ref is accepted. They're never auto-generated, never rewritten on navigation, and must be unique within a session. **Dashboard fix.** `packages/dashboard/src/types.ts` declared `TabInfo.index: number` but the daemon has been sending `tabId` (not `index`) since #892, making `tab.index` `undefined` and breaking the dashboard's close/switch buttons silently. Updated the TS types and usages to consume `tabId` (string) and optional `label`, restoring the dashboard's tab interactions. ## Surface - `cli/src/native/browser.rs`: `TabRef::parse` / `format_tab_id` / `is_valid_label` / `PageInfo.label` / `BrowserManager::resolve_tab_ref` / `BrowserManager::has_label`. `tab_new` gains an optional label argument with duplicate rejection. All JSON responses use the string form and include the label. - `cli/src/native/actions.rs`: scoped-command pre-dispatch and `handle_tab_{switch,close,new}` parse string refs and resolve to stable ids. - `cli/src/{flags,commands,main,output}.rs`: `--tab` / config `tab` are `String`; `tab` subcommand accepts `t<N>` or a label and supports `tab new --label <name> [url]`. All 52 `--help` entries updated. - `agent-browser.schema.json`: `tab` property type is now `string` with a pattern matching `t<N>` or label form. - `packages/dashboard`: `TabInfo.tabId: string` / `label?: string | null`; `closeTabAtom`/`switchTabAtom` take `tabRef: string`; component props updated. - Docs: README, docs site (`commands/` and `configuration/`), and the agent-facing skills reference rewritten with the new examples. ## Tests - Added `TabRef::parse` / `format_tab_id` / `is_valid_label` unit tests pinning the bare-integer rejection, the teaching error, label rules, and round-tripping. - Added `test_tab_switch_by_id` / `_by_label` / `test_tab_new_with_label` / `_with_label_and_url` / `_with_url_then_label` in `commands.rs`; rewrote `test_tab_unknown_subcommand_errors` since labels make `tab select` a legitimate ref. - Added `e2e_tab_new_with_label_can_be_switched_and_peeked`, `e2e_tab_new_with_duplicate_label_errors`, `e2e_tab_scoped_command_rejects_bare_integer`. - Migrated every existing tab e2e test (and one unit test) from integer `tabId` to the string form. `cargo fmt`, `cargo clippy -- -D warnings`, all 30 non-ignored tab unit tests, all 13 tab e2e tests, and `tsc --noEmit` on the dashboard all pass. * refactor(tabs): drop --tab scoped peek flag; keep t<N> ids and labels After fleshing out `--tab <id|label>` in the previous commits (scoped pre/post-dispatch save/restore, ref preservation, outer-tab-closed edge case, full e2e coverage), the machinery-to-value ratio makes the feature hard to justify. Nixing it now while nothing has shipped. ## Why - Every new daemon feature touching per-tab state has to reason about scoped-dispatch interleaving. `ScopedRestore`, pre/post-dispatch hooks, and the exclusion list add ongoing maintenance tax. - Three separate PRs (#892, #1249, and this one pre-nix) were needed to reach "works correctly." That's a smell. - `tab <id|label>` switch + labels already cover the legible multi-tab workflow case. - `--tab` vs `tab <id>` have opposite lifecycle semantics but look identical, teaching every agent two things where one would do. - "Non-disruptive peek" isn't actually race-free: the daemon does swap active tab during execution, so a concurrent client between pre- and post-dispatch sees the scoped tab as active. - Ref-based interaction with scoped tabs never worked ergonomically — refs are per-tab, so `--tab N click @e1` requires `@e1` to already be on tab N, which means a prior switch, which negates the peek. - Adding a feature back is easy; removing shipped API is hard. If per-tab caching (`HashMap<tab_id, RefMap>`) lands later, `--tab` can be reintroduced essentially for free. That's the right time. ## Removed - `--tab <id|label>` global flag (`cli/src/flags.rs`, `cli/src/main.rs`, all 52 `--help` entries in `cli/src/output.rs`). - `tab` property in `agent-browser.schema.json` and the config-options row in `docs/src/app/configuration/page.mdx`. - `ScopedRestore` struct, pre/post-dispatch save/restore in `execute_command` (`cli/src/native/actions.rs`). - `impl Default for RefMap` in `cli/src/native/element.rs` (only added for `mem::take` in the scoped machinery). - `e2e_tab_global_targeting`, `_snapshot`, `_snapshot_non_contiguous`, `e2e_tab_scoped_command_preserves_outer_tab_state`, `_isolates_refs_from_outer_tab`, `_restores_active_tab`, `_outer_tab_closed_mid_dispatch`. 590 lines. - The "When to use `--tab` vs `tab <id|label>`" sections in README, docs site, and skills reference. ## Kept - Stable tab ids (`t1`, `t2`, `t3`) with bare-integer rejection. - User-assigned labels (`tab new --label docs [url]`), with duplicate rejection and interchangeable use everywhere a tab ref is accepted. - `BrowserManager::{active_tab_id, has_tab_id, resolve_tab_ref, has_label}` accessors (still used by the remaining tab handlers). - `TabRef::parse`, `format_tab_id`, `is_valid_label` and their unit tests. - Dashboard TS fix (`TabInfo.tabId` + `label`). - `e2e_tab_close_with_tab_id_closes_active_tab` (renamed docstring to drop the gone exclusion-list reference). - `e2e_tab_new_with_label_can_be_switched_and_closed` (rewrite of the previous `_and_peeked` test — now exercises only switch and close). - `e2e_tab_switch_rejects_bare_integer` (rewrite targeting the `tab_switch` daemon handler rather than the removed scoped path). net: -900 lines across 12 files. `cargo fmt`, `cargo clippy -D warnings`, all 25 non-ignored tab unit tests, all 6 tab e2e tests, and `tsc --noEmit` on the dashboard all pass.
This commit is contained in:
Chris Tate
+105
-29
@@ -1011,31 +1011,51 @@ fn parse_command_inner(args: &[String], flags: &Flags) -> Result<Value, ParseErr
|
||||
|
||||
// === Tabs ===
|
||||
"tab" => {
|
||||
const VALID: &[&str] = &["list", "new", "close", "<id>"];
|
||||
match rest.first().copied() {
|
||||
Some("new") => {
|
||||
// Accepted forms:
|
||||
// tab new [url]
|
||||
// tab new --label <name> [url]
|
||||
// tab new [url] --label <name>
|
||||
let mut cmd = json!({ "id": id, "action": "tab_new" });
|
||||
if let Some(url) = rest.get(1) {
|
||||
cmd["url"] = json!(url);
|
||||
let mut i = 1;
|
||||
while i < rest.len() {
|
||||
match rest[i] {
|
||||
"--label" => {
|
||||
let name = rest.get(i + 1).ok_or(ParseError::MissingArguments {
|
||||
context: "tab new --label".to_string(),
|
||||
usage: "tab new --label <name> [url]",
|
||||
})?;
|
||||
cmd["label"] = json!(name);
|
||||
i += 2;
|
||||
}
|
||||
other if !other.starts_with("--") && cmd.get("url").is_none() => {
|
||||
cmd["url"] = json!(other);
|
||||
i += 1;
|
||||
}
|
||||
other => {
|
||||
return Err(ParseError::UnknownSubcommand {
|
||||
subcommand: other.to_string(),
|
||||
valid_options: &["--label", "<url>"],
|
||||
});
|
||||
}
|
||||
}
|
||||
}
|
||||
Ok(cmd)
|
||||
}
|
||||
Some("list") => Ok(json!({ "id": id, "action": "tab_list" })),
|
||||
Some("close") => {
|
||||
let mut cmd = json!({ "id": id, "action": "tab_close" });
|
||||
if let Some(tab_id) = rest.get(1).and_then(|s| s.parse::<i32>().ok()) {
|
||||
cmd["tabId"] = json!(tab_id);
|
||||
if let Some(tab_ref) = rest.get(1) {
|
||||
cmd["tabId"] = json!(tab_ref);
|
||||
}
|
||||
Ok(cmd)
|
||||
}
|
||||
Some(n) if n.parse::<i32>().is_ok() => {
|
||||
let tab_id = n.parse::<i32>().expect("already checked parse succeeds");
|
||||
Ok(json!({ "id": id, "action": "tab_switch", "tabId": tab_id }))
|
||||
}
|
||||
Some(sub) => Err(ParseError::UnknownSubcommand {
|
||||
subcommand: sub.to_string(),
|
||||
valid_options: VALID,
|
||||
}),
|
||||
Some(tab_ref) => Ok(json!({
|
||||
"id": id,
|
||||
"action": "tab_switch",
|
||||
"tabId": tab_ref,
|
||||
})),
|
||||
None => Ok(json!({ "id": id, "action": "tab_list" })),
|
||||
}
|
||||
}
|
||||
@@ -2342,7 +2362,6 @@ mod tests {
|
||||
fn default_flags() -> Flags {
|
||||
Flags {
|
||||
session: "test".to_string(),
|
||||
tab: None,
|
||||
json: false,
|
||||
headed: false,
|
||||
debug: false,
|
||||
@@ -2852,10 +2871,17 @@ mod tests {
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_tab_switch() {
|
||||
let cmd = parse_command(&args("tab 2"), &default_flags()).unwrap();
|
||||
fn test_tab_switch_by_id() {
|
||||
let cmd = parse_command(&args("tab t2"), &default_flags()).unwrap();
|
||||
assert_eq!(cmd["action"], "tab_switch");
|
||||
assert_eq!(cmd["tabId"], 2);
|
||||
assert_eq!(cmd["tabId"], "t2");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_tab_switch_by_label() {
|
||||
let cmd = parse_command(&args("tab docs"), &default_flags()).unwrap();
|
||||
assert_eq!(cmd["action"], "tab_switch");
|
||||
assert_eq!(cmd["tabId"], "docs");
|
||||
}
|
||||
|
||||
#[test]
|
||||
@@ -2866,23 +2892,57 @@ mod tests {
|
||||
|
||||
#[test]
|
||||
fn test_tab_close_with_id() {
|
||||
let cmd = parse_command(&args("tab close 2"), &default_flags()).unwrap();
|
||||
let cmd = parse_command(&args("tab close t2"), &default_flags()).unwrap();
|
||||
assert_eq!(cmd["action"], "tab_close");
|
||||
assert_eq!(cmd["tabId"], 2);
|
||||
assert_eq!(cmd["tabId"], "t2");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_tab_switch_sends_tab_id() {
|
||||
let cmd = parse_command(&args("tab 2"), &default_flags()).unwrap();
|
||||
assert_eq!(cmd["tabId"], 2);
|
||||
fn test_tab_close_with_label() {
|
||||
let cmd = parse_command(&args("tab close docs"), &default_flags()).unwrap();
|
||||
assert_eq!(cmd["action"], "tab_close");
|
||||
assert_eq!(cmd["tabId"], "docs");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_tab_sends_string_tab_id() {
|
||||
let cmd = parse_command(&args("tab t2"), &default_flags()).unwrap();
|
||||
assert!(
|
||||
cmd["tabId"].is_string(),
|
||||
"tabId must be a string, got: {:?}",
|
||||
cmd["tabId"]
|
||||
);
|
||||
assert!(cmd.get("index").is_none());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_tab_close_sends_tab_id() {
|
||||
let cmd = parse_command(&args("tab close 3"), &default_flags()).unwrap();
|
||||
assert_eq!(cmd["tabId"], 3);
|
||||
assert!(cmd.get("index").is_none());
|
||||
fn test_tab_new_with_label() {
|
||||
let cmd = parse_command(&args("tab new --label docs"), &default_flags()).unwrap();
|
||||
assert_eq!(cmd["action"], "tab_new");
|
||||
assert_eq!(cmd["label"], "docs");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_tab_new_with_label_and_url() {
|
||||
let cmd = parse_command(
|
||||
&args("tab new --label docs https://docs.example.com"),
|
||||
&default_flags(),
|
||||
)
|
||||
.unwrap();
|
||||
assert_eq!(cmd["action"], "tab_new");
|
||||
assert_eq!(cmd["label"], "docs");
|
||||
assert_eq!(cmd["url"], "https://docs.example.com");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_tab_new_with_url_then_label() {
|
||||
let cmd = parse_command(
|
||||
&args("tab new https://docs.example.com --label docs"),
|
||||
&default_flags(),
|
||||
)
|
||||
.unwrap();
|
||||
assert_eq!(cmd["url"], "https://docs.example.com");
|
||||
assert_eq!(cmd["label"], "docs");
|
||||
}
|
||||
|
||||
#[test]
|
||||
@@ -2892,14 +2952,30 @@ mod tests {
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_tab_unknown_subcommand_errors() {
|
||||
let result = parse_command(&args("tab select 3"), &default_flags());
|
||||
fn test_tab_unknown_flag_errors() {
|
||||
// Unknown flags on `tab new` must error instead of being silently
|
||||
// dropped. This protects against typos like `--labl` or `--new-tab`.
|
||||
let result = parse_command(
|
||||
&args("tab new --unknown-flag https://example.com"),
|
||||
&default_flags(),
|
||||
);
|
||||
assert!(
|
||||
result.is_err(),
|
||||
"tab select should error, not silently fall through to tab_list"
|
||||
"tab new with an unknown flag must error, got: {:?}",
|
||||
result
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_tab_non_keyword_treated_as_ref() {
|
||||
// After the shift to `t<N>`/label ids, non-keyword tokens (`select`,
|
||||
// `docs`, etc.) are valid label refs; `tab <something>` routes to
|
||||
// tab_switch and the runtime decides whether the label exists.
|
||||
let cmd = parse_command(&args("tab select"), &default_flags()).unwrap();
|
||||
assert_eq!(cmd["action"], "tab_switch");
|
||||
assert_eq!(cmd["tabId"], "select");
|
||||
}
|
||||
|
||||
// === Network ===
|
||||
|
||||
#[test]
|
||||
|
||||
Reference in New Issue
Block a user