fix: reuse existing blank tab in browser harness#478
Conversation
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e3c0396afc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if url != "about:blank": | ||
| goto_url(url) | ||
| except Exception: | ||
| try: cdp("Target.closeTarget", targetId=tid) |
There was a problem hiding this comment.
When goto_url() raises after switch_tab(tid) has already sent set_session, this cleanup closes the tab that the daemon is now attached to but leaves self.session/target_id pointing at the closed target. In callers that catch the navigation error and continue in the same harness invocation, the next command starts from a stale session or reattaches to whichever page attach_first_page() picks, instead of returning to the tab that was active before new_tab(). Capture the previous tab/session and switch back after closing the failed target.
Useful? React with 👍 / 👎.
iiai-lab
force-pushed
the
fix/reuse-existing-blank-tab
branch
from
e3c0396 to
7594909
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a888566bf6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| def is_reusable_empty_page(t): | ||
| """True for visible empty tabs we can attach to instead of creating another one.""" | ||
| return t["type"] == "page" and t.get("url", "") in {"about:blank", "about:newtab", "chrome://newtab/"} |
There was a problem hiding this comment.
When Chrome reports the only empty tab as about:newtab or chrome://newtab/, this line now makes the daemon attach to that page during startup. The documented first navigation still goes through helpers.new_tab(url), but that helper only reuses "", about:blank, or about:blank#..., so it immediately creates another target instead of navigating the selected new-tab page. In fresh Chrome sessions this leaves the original new-tab page behind and reintroduces the tab spam this change is trying to avoid; either keep these URLs out of this reusable set or teach new_tab() to navigate them too.
Useful? React with 👍 / 👎.
iiai-lab
force-pushed
the
fix/reuse-existing-blank-tab
branch
from
a888566 to
6583203
Compare
|
@codex review |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found and verified against the latest diff
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/browser_harness/daemon.py">
<violation number="1" location="src/browser_harness/daemon.py:191">
P2: New-tab reuse may miss common CDP new-tab URL variants because the allowlist only accepts `chrome://newtab/`.</violation>
<violation number="2" location="src/browser_harness/daemon.py:191">
P2: Inconsistent reusable-page criteria between `is_reusable_empty_page()` and `new_tab()`. This function treats `about:newtab` and `chrome://newtab/` as reusable (so the daemon attaches to them at startup), but `helpers.new_tab(url)` only reuses tabs with URL `""`, `"about:blank"`, or `"about:blank#..."`. In a fresh Chrome session where the sole tab is `about:newtab`, the daemon attaches to it, but the next `new_tab(url)` call won't navigate it—it will create another target, leaving the original newtab page behind. Either remove `about:newtab`/`chrome://newtab/` from this set or extend the reuse check in `new_tab()` to cover them as well.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
|
|
||
| def is_reusable_empty_page(t): | ||
| """True for visible empty tabs we can attach to instead of creating another one.""" | ||
| return t["type"] == "page" and t.get("url", "") in {"about:blank", "about:newtab", "chrome://newtab/"} |
There was a problem hiding this comment.
P2: Inconsistent reusable-page criteria between is_reusable_empty_page() and new_tab(). This function treats about:newtab and chrome://newtab/ as reusable (so the daemon attaches to them at startup), but helpers.new_tab(url) only reuses tabs with URL "", "about:blank", or "about:blank#...". In a fresh Chrome session where the sole tab is about:newtab, the daemon attaches to it, but the next new_tab(url) call won't navigate it—it will create another target, leaving the original newtab page behind. Either remove about:newtab/chrome://newtab/ from this set or extend the reuse check in new_tab() to cover them as well.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/browser_harness/daemon.py, line 191:
<comment>Inconsistent reusable-page criteria between `is_reusable_empty_page()` and `new_tab()`. This function treats `about:newtab` and `chrome://newtab/` as reusable (so the daemon attaches to them at startup), but `helpers.new_tab(url)` only reuses tabs with URL `""`, `"about:blank"`, or `"about:blank#..."`. In a fresh Chrome session where the sole tab is `about:newtab`, the daemon attaches to it, but the next `new_tab(url)` call won't navigate it—it will create another target, leaving the original newtab page behind. Either remove `about:newtab`/`chrome://newtab/` from this set or extend the reuse check in `new_tab()` to cover them as well.</comment>
<file context>
@@ -186,6 +186,11 @@ def is_real_page(t):
+def is_reusable_empty_page(t):
+ """True for visible empty tabs we can attach to instead of creating another one."""
+ return t["type"] == "page" and t.get("url", "") in {"about:blank", "about:newtab", "chrome://newtab/"}
+
+
</file context>
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review The latest head teaches helpers.new_tab() to reuse about:newtab/chrome://newtab/ as well, with a regression test. Please re-check the current head. |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/browser_harness/daemon.py">
<violation number="1" location="src/browser_harness/daemon.py:191">
P2: Inconsistent reusable-page criteria between `is_reusable_empty_page()` and `new_tab()`. This function treats `about:newtab` and `chrome://newtab/` as reusable (so the daemon attaches to them at startup), but `helpers.new_tab(url)` only reuses tabs with URL `""`, `"about:blank"`, or `"about:blank#..."`. In a fresh Chrome session where the sole tab is `about:newtab`, the daemon attaches to it, but the next `new_tab(url)` call won't navigate it—it will create another target, leaving the original newtab page behind. Either remove `about:newtab`/`chrome://newtab/` from this set or extend the reuse check in `new_tab()` to cover them as well.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
Prefer attaching to an existing empty tab before creating a new target, and close newly-created tabs if navigation fails. Update usage docs and add regression coverage.
iiai-lab
force-pushed
the
fix/reuse-existing-blank-tab
branch
from
6583203 to
2a67bf5
Compare
|
@codex review Updated current head also recovers to a fallback attached tab when previous current_tab cannot be captured. Please review current head. |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Summary
Test plan
Summary by cubic
Reuse existing blank/new-tab pages to avoid tab spam. On navigation failure, close the new tab and restore the previous tab, or fall back to a safe tab. Docs now prefer
ensure_real_tab()+goto_url(), with regression tests.Daemon.attach_first_pagenow attaches to existingabout:blank/about:newtab/chrome://newtab/pages before creating one.helpers.new_tab()reuses a current blank/new-tab when possible; on navigation failure it closes the created tab, restores the previous tab if available, otherwise switches to an existing tab or creates a fresh blank tab.Written for commit 2a67bf5. Summary will update on new commits.