fix for windows extension autoload missing side panel#166
Conversation
volodymyr-narada
changed the title
| side_panel_page = launch_browser_result.side_panel_page | ||
|
|
||
| await self._fix_download_behavior(side_panel_page) | ||
| if side_panel_page is not None: |
There was a problem hiding this comment.
This conditional logic causes a problem: I tested the hello world example on Windows, and this check caused the fix download behavior call to be skipped, and the files the SDK downloaded became UUID gibberish instead of real downloads.
|
Could you try updating the Playwright version used by the Otherwise, we need to fully switch to always using CDP target for the side panel page to make sure tasks like fixing download behavior are still always performed. Otherwise, the user gets a partially functional environment. |
zizhengtai
left a comment
zizhengtai
left a comment
There was a problem hiding this comment.
I found three issues that I think should be addressed before merging. Details are inline.
| side_panel_page, | ||
| has_side_panel_target, | ||
| ) = await _find_side_panel_page_or_target(browser, side_panel_url) | ||
| if side_panel_page is not None or has_side_panel_target: |
There was a problem hiding this comment.
[P1] Keep reset_agent_state() working for target-only side panels. When this branch exits because has_side_panel_target is true while side_panel_page is None, initialization succeeds with no side-panel entry in self._context.pages. reset_agent_state() still performs next(...) over those pages, so in the exact Windows state this PR enables it deterministically raises RuntimeError: coroutine raised StopIteration. Please retain the target ID and reload it over CDP (or otherwise make reset independent of a Playwright Page) and add a target-only initialization/reset test.
| try: | ||
| cdp_session = await browser.new_browser_cdp_session() | ||
| await cdp_session.send( | ||
| "Browser.setDownloadBehavior", {"behavior": "default"} |
There was a problem hiding this comment.
[P2] Apply the download reset to the matched browser context. Browser.setDownloadBehavior without browserContextId affects only the default context. Because _find_page_by_url() deliberately scans every context, this command can succeed for context 0, skip the page-scoped fallback, and leave a side panel found in another context using Playwright's allowAndName behavior (UUID filenames). When a Page exists, use its page-scoped session; for a target-only match, retain targetInfo.browserContextId and pass it here.
| ) | ||
| ) | ||
| ) | ||
| except NaradaTimeoutError: |
There was a problem hiding this comment.
[P2] Put an overall bound on initialization retries. This 30-second initialization wait is now retried through the ten-attempt CDP loop. With the 15-second navigation timeout and three-second sleep, a permanently stalled initialization page can keep start() running for roughly 5.5–7.75 minutes instead of failing after about 30 seconds. Please use a smaller, independent initialization retry count or one overall deadline, and add an always-timeout test that asserts the bound.
|
Implemented the four follow-up fixes in
I also did a full self-review of the branch and added regression coverage for response/lifecycle event ordering, stale lifecycle filtering, target detachment, timeout/listener cleanup, rediscovery, retry bounds, and default/non-default download behavior. Validation:
|
zizhengtai
left a comment
zizhengtai
left a comment
There was a problem hiding this comment.
Re-reviewed the latest head (b832693) after the follow-up fixes. The four issues are addressed, the full self-review found no remaining findings, and both local validation and all PR checks pass.
zizhengtai
deleted the
volodymyr/windows-extension-autoload-side-panel-fix
branch
2 participants
Summary
Fixes local browser initialization when Chrome opens the Narada side panel, but Playwright does not expose that side panel as a
Page.On Windows, we observed Chrome reporting the side panel through raw CDP targets:
while
browser.contexts[*].pagesonly contained the initialization page. The SDK was waiting for the side panel to appear as a Playwright page, so initialization could hang or time out even though the side panel was already open and usable.Solution
This PR keeps
create_side_panel_url(...)as the source of truth for the expected side panel URL, then checks for that exact URL in two places:Target.getTargets.If the side panel is exposed as a Playwright
Page, initialization continues as before. If it is only visible as a raw CDP target, initialization is still accepted using the capturedbrowserWindowId.Download behavior is fixed in both cases. The SDK now prefers browser-level CDP via
Browser.setDownloadBehavior, which does not require the side panel to be exposed as a PlaywrightPage. If browser-level CDP is unavailable and the side panel is available as a PlaywrightPage, the SDK falls back to the existing page-levelPage.setDownloadBehaviorpath. If neither path is available, initialization fails rather than silently continuing with broken downloads.The init-page timeout path also retries by reloading the initialization page, which helps with intermittent “Initializing…” stalls.
Tests