Expect new page and fix js windows handling#293
Merged
mshriver merged 1 commit intoRedHatQE:mainfrom Nov 6, 2025
Merged
Conversation
Reviewer's GuideThis PR introduces a new expect_new_page context manager that leverages Playwright’s expect_page API to reliably wait for and wrap newly opened pages, refactors all_browsers to improve cleanup and best-effort detection of pages in transition, extends switch_to and close_browser to accept the new context manager’s return values, and adds an end-to-end test for JS-triggered window openings. Sequence diagram for expect_new_page context manager usagesequenceDiagram
actor User
participant WindowManager
participant _BrowserWaiter
participant PlaywrightContext
participant Browser
participant Page
User->>WindowManager: with expect_new_page() as new_browser
WindowManager->>_BrowserWaiter: create _BrowserWaiter
_BrowserWaiter->>PlaywrightContext: expect_page(timeout)
PlaywrightContext-->>_BrowserWaiter: returns page_info
User->>Browser: browser.click("a[target='_blank']")
_BrowserWaiter->>Page: wait_for_load_state("domcontentloaded")
_BrowserWaiter->>WindowManager: _wrap_page(new_page)
WindowManager-->>Browser: returns wrapped Browser
_BrowserWaiter-->>User: yields new_browser (wrapped Browser)
User->>WindowManager: close_browser(new_browser)
WindowManager->>Browser: close
Class diagram for new and updated window management classesclassDiagram
class WindowManager {
+expect_new_page(timeout: float = 5.0)
+all_browsers: List[Browser]
+all_pages: List[Page]
+switch_to(browser_or_page: Union[Browser, Page, BrowserWaiter])
+close_browser(browser_or_page: Optional[Union[Browser, Page, BrowserWaiter]] = None)
-_wrap_page(page: Page)
-_context
-_browsers
}
class _BrowserWaiter {
+__enter__() _BrowserWaiter
+__exit__(*args)
+__getattr__(name: str)
+__eq__(other: Any)
-window_manager: WindowManager
-context
-timeout_ms: int
-page_info: Optional[Any]
-_browser: Optional[Browser]
-_initial_browsers: Optional[Set[Browser]]
}
WindowManager "1" -- "*" _BrowserWaiter : creates
_BrowserWaiter "1" -- "1" WindowManager : references
_BrowserWaiter "1" -- "1" Browser : wraps
WindowManager "*" -- "1" Browser : manages
Browser "1" -- "1" Page : wraps
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/widgetastic/browser.py:1761-1770` </location>
<code_context>
+
+ new_page = None
+ self.page_info.__exit__(*args)
+ try:
+ new_page = self.page_info.value
+ except AttributeError:
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Silent failure on TimedOutError may obscure issues.
Logging a debug message when TimedOutError is caught will make it easier to identify unexpected failures in new page detection.
Suggested implementation:
```python
if new_page:
# Wait for page to load and wrap it
try:
new_page.wait_for_load_state("domcontentloaded", timeout=5000)
except TimedOutError:
logger.debug("Timed out waiting for new page to load (domcontentloaded).")
except Exception:
pass
self._browser = self.window_manager._wrap_page(new_page)
```
```python
import logging
from playwright.sync_api import TimedOutError
logger = logging.getLogger(__name__)
```
If `logger` or `TimedOutError` are already imported elsewhere in the file, you can omit those import lines. Make sure the logging configuration is set up somewhere in your application to see debug messages.
</issue_to_address>
### Comment 2
<location> `testing/test_window_manager.py:365-374` </location>
<code_context>
+def test_js_window_open_detection(
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding negative test cases for failed window/tab opening.
Adding tests for scenarios where window/tab opening fails, such as blocked popups or links that do not open new pages, will help confirm that `expect_new_page` handles timeouts and errors as expected.
Suggested implementation:
```python
def test_js_window_open_detection(
isolated_window_manager,
popup_test_page_url,
):
"""Test that new tabs/popups opened via JavaScript or links are automatically detected.
This test verifies that WindowManager correctly detects new pages opened through:
- JavaScript window.open() for popup windows
- JavaScript window.open() for new tabs
- HTML anchor tags with target="_blank"
It also verifies negative cases where no new window/tab is opened.
"""
# Positive test cases (existing)
# ... (existing positive test code here)
# Negative test case 1: No action taken, expect_new_page should timeout or raise
with isolated_window_manager.current.page.expect_new_page(timeout=1000) as new_page_info:
pass # No action, should not open a new page
try:
new_page_info.value
assert False, "expect_new_page should have timed out when no new page is opened"
except Exception:
pass # Expected
# Negative test case 2: Click a link without target="_blank", should not open a new page
page = isolated_window_manager.current.page
page.goto(popup_test_page_url)
# Assume there is a link with id 'no_blank_link' that does not open a new tab
try:
with page.expect_new_page(timeout=1000) as new_page_info:
page.click("#no_blank_link")
new_page_info.value
assert False, "expect_new_page should have timed out for link without target=_blank"
except Exception:
pass # Expected
# Negative test case 3: Simulate popup blocked by browser
# This may require mocking or simulating browser settings, but we can try a JS call that is known to be blocked
try:
with page.expect_new_page(timeout=1000) as new_page_info:
page.evaluate("window.open('about:blank', '_blank', 'popup,noopener')")
new_page_info.value
assert False, "expect_new_page should have timed out if popup is blocked"
except Exception:
pass # Expected
```
- You may need to adjust the selectors (e.g., "#no_blank_link") to match actual elements in your test page.
- If your test framework uses a different way to handle timeouts or exceptions, adapt the try/except blocks accordingly.
- Ensure the positive test cases are still present and run before the negative cases.
</issue_to_address>
### Comment 3
<location> `testing/test_window_manager.py:398-432` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid loops in tests. ([`no-loop-in-tests`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/no-loop-in-tests))
<details><summary>Explanation</summary>Avoid complex code, like loops, in test functions.
Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
* loops
* conditionals
Some ways to fix this:
* Use parametrized tests to get rid of the loop.
* Move the complex logic into helpers.
* Move the complex part into pytest fixtures.
> Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / [Don't Put Logic in Tests](https://abseil.io/resources/swe-book/html/ch12.html#donapostrophet_put_logic_in_tests)
</details>
</issue_to_address>
### Comment 4
<location> `src/widgetastic/browser.py:1714` </location>
<code_context>
def expect_new_page(self, timeout: float = 5.0):
"""Context manager to wait for a new page and return the wrapped Browser instance.
This method uses Playwright's native `expect_page()` to wait for a new page
to be created. Use this when you know an action (like clicking a link with
target="_blank" or a button that calls window.open()) will open a new page.
The context manager automatically waits for the page to load (domcontentloaded).
Args:
timeout: Maximum time to wait for the new page in seconds (default: 5.0)
Yields:
Browser: The wrapped Browser instance for the new page
Example:
.. code-block:: python
# Get Browser directly - returns actual Browser instance
with window_manager.expect_new_page() as new_browser:
browser.click("a[target='_blank']")
# new_browser is the wrapped Browser instance, ready to use
assert new_browser.title == "New Page"
new_browser.element("#submit").click()
window_manager.close_browser(new_browser)
# Or with a longer timeout
with window_manager.expect_new_page(timeout=10.0) as popup_browser:
browser.click("#open-popup-button")
popup_browser.element("#confirm").click()
"""
class _BrowserWaiter:
"""Simple context manager that waits for page and returns wrapped Browser."""
def __init__(self, window_manager: "WindowManager", timeout_ms: int) -> None:
self.window_manager = window_manager
self.context = window_manager._context
self.timeout_ms = timeout_ms
self.page_info: Optional[Any] = None
self._browser: Optional[Browser] = None
self._initial_browsers: Optional[Set[Browser]] = None
def __enter__(self) -> "_BrowserWaiter":
# Capture initial browsers before setting up expectation
self._initial_browsers = set(self.window_manager.all_browsers)
self.page_info = self.context.expect_page(timeout=self.timeout_ms)
self.page_info.__enter__()
return self
def __exit__(self, *args: Any) -> None:
# Exit Playwright's context manager to get the Page/Browser
if not self.page_info:
# If page_info was never set, something went wrong in __enter__
raise RuntimeError(
"expect_new_page context manager was not properly initialized. "
"This should not happen."
)
new_page = None
self.page_info.__exit__(*args)
try:
new_page = self.page_info.value
except AttributeError:
# value might not be available in some edge cases
pass
if new_page:
# Wait for page to load and wrap it
try:
new_page.wait_for_load_state("domcontentloaded", timeout=5000)
except Exception:
pass
self._browser = self.window_manager._wrap_page(new_page)
else:
# expect_page succeeded but value is None.This can happen if the event handler already wrapped the page
# Try to find it by comparing browser sets
if self._initial_browsers is None:
raise RuntimeError(
"expect_new_page context manager was not properly initialized. "
"This should not happen."
)
current_browsers = set(self.window_manager.all_browsers)
new_browsers = current_browsers - self._initial_browsers
if new_browsers:
self._browser = next(iter(new_browsers))
else:
# No new page found - this shouldn't happen if expect_page succeeded
raise RuntimeError(
"expect_page succeeded but no new page was found. "
"This may indicate a timing issue or the page was closed immediately."
)
def __getattr__(self, name: str) -> Any:
"""Delegate all attribute access to the wrapped Browser."""
if self._browser is None:
raise RuntimeError(
"Browser is not available yet. Access it after the 'with' block exits."
)
return getattr(self._browser, name)
def __eq__(self, other: Any) -> bool:
"""Compare by page identity for 'in' checks to work."""
if self._browser is None:
return False
if isinstance(other, Browser):
return self._browser.page is other.page
return False
return _BrowserWaiter(self, int(timeout * 1000))
</code_context>
<issue_to_address>
**issue (code-quality):** Use named expression to simplify assignment and conditional ([`use-named-expression`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/use-named-expression/))
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #293 +/- ##
==========================================
- Coverage 93.50% 92.77% -0.74%
==========================================
Files 19 19
Lines 2617 2698 +81
==========================================
+ Hits 2447 2503 +56
- Misses 170 195 +25
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
digitronik
force-pushed
the
js_windows_hanlding
branch
3 times, most recently
from
255556a to
9d49373
Compare
digitronik
force-pushed
the
js_windows_hanlding
branch
from
9d49373 to
3bee93a
Compare
mshriver
approved these changes
Nov 6, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Introduce a context manager to await new pages and refine WindowManager’s page tracking by improving cleanup, detection and support for BrowserWaiter in navigation and closure methods
New Features:
Enhancements:
Tests: