From d0f0807f7694730c17d41de99a9ba4aabd54a788 Mon Sep 17 00:00:00 2001 From: Matt Van Horn Date: Tue, 26 May 2026 21:10:33 -0700 Subject: [PATCH] fix: close_tab() recovers session state when closing the currently attached tab Implements the work described in 2026-05-26-115-fix-close-tab-dangling-session-state-plan.md. Closes #379. --- src/browser_harness/helpers.py | 30 ++++++- tests/unit/test_helpers.py | 154 +++++++++++++++++++++++++++++++++ 2 files changed, 181 insertions(+), 3 deletions(-) diff --git a/src/browser_harness/helpers.py b/src/browser_harness/helpers.py index 2014887b..b4335a02 100644 --- a/src/browser_harness/helpers.py +++ b/src/browser_harness/helpers.py @@ -326,11 +326,35 @@ def new_tab(url="about:blank"): def close_tab(target=None): """Close a tab. If `target` is omitted, closes the currently attached tab. - Accepts a raw targetId string or a dict from list_tabs()/current_tab().""" + Accepts a raw targetId string or a dict from list_tabs()/current_tab(). + Closing the attached tab first switches the harness to another real tab, or + a fresh about:blank tab if there is no other real tab.""" target_id = target.get("targetId") if isinstance(target, dict) else target if target_id is None: - target_id = current_tab()["targetId"] - cdp("Target.closeTarget", targetId=target_id) + cur = current_tab() + target_id = cur["targetId"] + else: + try: + cur = current_tab() + except Exception: + cur = None + if cur is None: + cdp("Target.closeTarget", targetId=target_id) + return + if target_id != cur["targetId"]: + cdp("Target.closeTarget", targetId=target_id) + return + + other_tabs = [t for t in list_tabs(include_chrome=False) if t["targetId"] != target_id] + if other_tabs: + switch_tab(other_tabs[0]) + else: + new_tab() + try: + cdp("Target.closeTarget", targetId=target_id) + except Exception: + switch_tab(cur) + raise def ensure_real_tab(): diff --git a/tests/unit/test_helpers.py b/tests/unit/test_helpers.py index 4a45ee07..498cd721 100644 --- a/tests/unit/test_helpers.py +++ b/tests/unit/test_helpers.py @@ -77,6 +77,160 @@ def fake_cdp(method, **kwargs): helpers.page_info() +# --- tabs --- + +class _FakeTabs: + def __init__(self, tabs, current): + self.tabs = [dict(t) for t in tabs] + self.session_target_id = current + self.session_id = f"session-{current}" + self.closed = [] + self.fail_close = False + self.next_tab = 1 + + def cdp(self, method, session_id=None, **params): + if method == "Target.getTargets": + return { + "targetInfos": [ + {"type": "page", **t} + for t in self.tabs + ] + } + if method == "Target.activateTarget": + return {} + if method == "Target.attachToTarget": + target_id = params["targetId"] + return {"sessionId": f"session-{target_id}"} + if method == "Target.createTarget": + target_id = f"new-tab-{self.next_tab}" + self.next_tab += 1 + self.tabs.append({"targetId": target_id, "url": params["url"], "title": ""}) + return {"targetId": target_id} + if method == "Target.closeTarget": + if self.fail_close: + raise RuntimeError("close failed") + target_id = params["targetId"] + self.closed.append(target_id) + self.tabs = [t for t in self.tabs if t["targetId"] != target_id] + return {"success": True} + if method == "Runtime.evaluate": + expression = params["expression"] + if "JSON.stringify" in expression: + tab = self._current_tab() + value = ( + f'{{"url":"{tab["url"]}","title":"{tab["title"]}",' + '"w":800,"h":600,"sx":0,"sy":0,"pw":800,"ph":600}' + ) + return {"result": {"value": value}} + if expression == "1+1": + return {"result": {"value": 2}} + return {"result": {"value": None}} + raise AssertionError(f"unexpected CDP method: {method}") + + def send(self, req): + meta = req.get("meta") + if meta == "current_tab": + tab = self._current_tab() + return {"targetId": tab["targetId"], "url": tab["url"], "title": tab["title"]} + if meta == "set_session": + self.session_id = req["session_id"] + self.session_target_id = req["target_id"] + return {"session_id": self.session_id} + if meta == "session": + return {"session_id": self.session_id} + if meta == "pending_dialog": + return {} + raise AssertionError(f"unexpected IPC request: {req}") + + def _current_tab(self): + for tab in self.tabs: + if tab["targetId"] == self.session_target_id: + return tab + raise RuntimeError(f"not attached to live tab: {self.session_target_id}") + + +def test_close_tab_current_switches_to_surviving_tab(): + browser = _FakeTabs( + [ + {"targetId": "survivor", "url": "https://survivor.example/", "title": "Survivor"}, + {"targetId": "current", "url": "https://current.example/", "title": "Current"}, + ], + current="current", + ) + + with patch("browser_harness.helpers.cdp", side_effect=browser.cdp), \ + patch("browser_harness.helpers._send", side_effect=browser.send): + helpers.close_tab() + + assert helpers.current_tab()["targetId"] == "survivor" + assert helpers.js("1+1") == 2 + + assert browser.closed == ["current"] + assert browser.session_target_id == "survivor" + assert browser.session_id == "session-survivor" + + +def test_close_tab_current_opens_blank_when_no_other_real_tab(): + browser = _FakeTabs( + [{"targetId": "current", "url": "https://current.example/", "title": "Current"}], + current="current", + ) + + with patch("browser_harness.helpers.cdp", side_effect=browser.cdp), \ + patch("browser_harness.helpers._send", side_effect=browser.send): + helpers.close_tab() + + assert helpers.current_tab()["targetId"] == "new-tab-1" + assert helpers.current_tab()["url"] == "about:blank" + assert helpers.page_info()["url"] == "about:blank" + + assert browser.closed == ["current"] + assert browser.session_target_id == "new-tab-1" + assert browser.session_id == "session-new-tab-1" + + +def test_close_tab_explicit_non_current_leaves_session_unchanged(): + browser = _FakeTabs( + [ + {"targetId": "current", "url": "https://current.example/", "title": "Current"}, + {"targetId": "other", "url": "https://other.example/", "title": "Other"}, + ], + current="current", + ) + + with patch("browser_harness.helpers.cdp", side_effect=browser.cdp), \ + patch("browser_harness.helpers._send", side_effect=browser.send): + helpers.close_tab(target="other") + + assert helpers.current_tab()["targetId"] == "current" + + assert browser.closed == ["other"] + assert browser.session_target_id == "current" + assert browser.session_id == "session-current" + + +def test_close_tab_current_rolls_back_session_when_close_fails(): + browser = _FakeTabs( + [ + {"targetId": "survivor", "url": "https://survivor.example/", "title": "Survivor"}, + {"targetId": "current", "url": "https://current.example/", "title": "Current"}, + ], + current="current", + ) + browser.fail_close = True + + with patch("browser_harness.helpers.cdp", side_effect=browser.cdp), \ + patch("browser_harness.helpers._send", side_effect=browser.send): + with pytest.raises(RuntimeError, match="close failed"): + helpers.close_tab() + + assert helpers.current_tab()["targetId"] == "current" + + assert browser.closed == [] + assert browser.session_target_id == "current" + assert browser.session_id == "session-current" + + # --- fill_input --- def test_fill_input_focuses_types_and_fires_events():