Skip to content

Conversation

@Ahmed-Tawfik94
Copy link
Collaborator

@Ahmed-Tawfik94 Ahmed-Tawfik94 commented Sep 17, 2025

Summary

This change fixes an issue where the viewport settings (viewport_width and viewport_height from BrowserConfig) were not being passed to managed browsers. Previously, managed browsers launched without the --window-size flag, causing them to ignore the specified viewport and default to their own window size. Now, the flag is added in ManagedBrowser._get_browser_args() for Chromium browsers, ensuring consistent viewport enforcement in managed mode.

Fixes the user's reported issue with use_managed_browser not respecting viewport settings.
#1490

List of files changed and why

  • browser_manager.py: Updated the _get_browser_args() method in the ManagedBrowser class to append the --window-size={width},{height} flag when viewport_width and viewport_height are set in browser_config. This ensures managed browsers (launched via CLI) receive the viewport dimensions, matching the behavior for non-managed browsers in BrowserManager._build_browser_args().

How Has This Been Tested?

  • Run the test_pad.py script with use_managed_browser=True and a specified viewport (e.g., {"width": 900, "height": 720}).
  • Verify the browser window size matches the config by inspecting the launched browser (e.g., via dev tools or window properties).
  • Add debug logging in _get_browser_args() to print the full args list and confirm the --window-size flag is included.
  • Test with use_managed_browser=False to ensure no regression in non-managed mode.
  • No automated tests were added, but manual verification shows the viewport is now applied correctly in managed mode.

Checklist:

  • My code follows the style guidelines of this project (uses existing patterns for flag appending).
  • I have performed a self-review of my own code (checked for syntax, logic, and integration with existing code).
  • I have commented my code, particularly in hard-to-understand areas (added inline comment explaining the viewport flag addition).
  • I have made corresponding changes to the documentation (not applicable for this internal fix; consider updating if viewport behavior is documented elsewhere).
  • I have added/updated unit tests that prove my fix is effective or that my feature works (manual testing performed; unit tests could be added in future for ManagedBrowser).
  • New and existing unit tests pass locally with my changes (assuming existing tests cover browser launch; no new failures introduced).

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 17, 2025

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

✨ Finishing touches
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/viewport_in_managed_browser

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@ntohidi ntohidi changed the title #1490 feat(ManagedBrowser): add viewport size configuration for browser launch feat(ManagedBrowser): add viewport size configuration for browser launch Sep 18, 2025
]
if self.headless:
flags.append("--headless=new")
# Add viewport flag if specified in config
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Ahmed-Tawfik94 why only if self.browser_type == "chromium":? why not for all browser types?

@ntohidi
Copy link
Collaborator

ntohidi commented Oct 6, 2025

@Ahmed-Tawfik94 test this on firefox too

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants