[Task-2247] Named Browser Sessions For Dual-Driver Browser Instances#671
[Task-2247] Named Browser Sessions For Dual-Driver Browser Instances#671nasif-z wants to merge 13 commits intoplaywright-asyncfrom
Conversation
Similar to Selenium open browser action
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e8e79d1b04
ℹ️ 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".
|
@nasif-z check the comments |
🔎 ZeuZ PR ReviewOpen the full report in ZeuZ: Review findings and apply suggestions
Agent breakdown→ General ReviewStatus: ✅ Completed The PR adds named browser sessions and per-session CDP ports, but the session plumbing is inconsistent and likely incorrect: session state handling duplicates logic in multiple places, relies on globals in a helper, and introduces likely runtime bugs (undefined variables, wrong teardown semantics, and incomplete/possibly broken new utils.py content). → Security ReviewStatus: ✅ Completed The PR adds named dual-driver browser sessions and introduces session switching/teardown plus unique remote-debugging ports. Main security risks are session-name misuse leading to unsafe resource lookup/teardown and potentially unsafe debug-port/CDP handling; there are also robustness issues that can cause cross-session data leakage or unintended teardown. → Architecture ReviewStatus: ✅ Completed High-level: the PR introduces cross-session handling for Playwright/Selenium and attempts to isolate CDP/remote-debug ports per session. However, it adds significant global-state mutation and relies on a new shared-session utility file without fully validating consistency, lifecycle, and parsing semantics—creating long-term fragility and potential runtime errors. → Performance ReviewStatus: ✅ Completed PR introduces per-session browser/session handling but adds multiple hotspots that can become inefficient and error-prone at scale (session parsing repeated per action, expensive list/dict scans during teardown, and repeated get_session/port-hash recomputation). Main performance risk is O(n) scans in hot teardown logic and redundant per-call parsing/derivation.
|
This PR introduces session-based browser isolation to allow testers to easily spawn fresh browser windows and intuitively interact with them individually. It follows the same session model used in database actions.
Changes
browser_sessionsshared variable to hold necessary browser driver information whenever opening new browser windows. By default, generates abrowser_sessionnameddefault.sessionparameter to only execute the actions on specific instances without affecting others.Backwards Compatibility
sessionparameter is omitted, the framework defaults to the legacy single-instance behavior.driveridsupport is maintained for Selenium.Usage
Add row in action in this format:
session|optional parameter|session name (string)1. Selenium Multi-Session
Pass a unique string to the
sessionparameter to spawn separate windows.2. Apply action on specific session
3. Specific Session Cleanup
4. Without "session" parameter
Omitting the
sessionparameter in step/action data will preserve the behavior as it was before the addition of this feature.