Skip to content

Conversation

@mzyndul
Copy link
Contributor

@mzyndul mzyndul commented Jan 25, 2026

User description

🔗 Related Issues

💥 What does this PR do?

Adds type annotations to the remote WebDriver module and related files to improve static type checking and IDE support.

🔧 Implementation Notes

  • Used cast() for return values from execute() since the underlying method returns dict[str, Any] and the actual types are known at each call site
  • Added # type: ignore comments sparingly where strict typing would require significant refactoring (e.g., CDP module dynamic imports, optional dict access chains)
  • For decorators, used TypeVar("F", bound=Callable[..., Any]) pattern to preserve function signatures
  • Changed ScriptKey._id to always be str (converting UUID to string) to maintain consistent typing and proper hashability

💡 Additional Considerations

  • These changes are additive and should not affect runtime behavior
  • The # type: ignore comments in bidi_connection() and _get_cdp_details() are due to the dynamically imported cdp module - a future improvement could add proper type stubs for the CDP module
  • Cookie methods use Any return type because the WebDriver protocol returns complex nested structures that vary by implementation

🔄 Types of changes

  • Cleanup (formatting, renaming)

PR Type

Enhancement


Description

  • Add comprehensive type hints to remote WebDriver module

  • Improve static type checking and IDE support across related modules

  • Use cast() for execute() return values with known types

  • Add TypeVar pattern for decorator function signature preservation

  • Convert ScriptKey._id to always be str for consistent typing


File Walkthrough

Relevant files
Enhancement
webdriver.py
Add comprehensive type hints to WebDriver class                   

py/selenium/webdriver/remote/webdriver.py

  • Added type hints to method signatures and return types throughout the
    WebDriver class
  • Used cast() for execute() return values where actual types are known
    (title, current_url, page_source, etc.)
  • Added type hints for parameters in methods like set_window_size,
    get_window_size, add_cookie, pin_script
  • Added type: ignore comments for dynamic cdp module imports and
    optional dict access chains
  • Improved typing for window/position/size related methods with
    dict[str, int] and dict[str, Any] returns
  • Added type hints for file_detector_context generator and
    bidi_connection async generator
+78/-58 
script_key.py
Add type hints and improve ScriptKey hashability                 

py/selenium/webdriver/remote/script_key.py

  • Added __future__ annotations import for forward references
  • Added type hints to __init__ method with str | None parameter
  • Changed _id to always be str by converting UUID to string
  • Added __hash__ method to make ScriptKey properly hashable
  • Added type hints to id property and __eq__ method
+11/-4   
virtual_authenticator.py
Add type hints to virtual authenticator decorators             

py/selenium/webdriver/common/virtual_authenticator.py

  • Added __future__ annotations import for forward references
  • Added TypeVar pattern for decorator functions to preserve signatures
  • Added type hints to required_chromium_based_browser and
    required_virtual_authenticator decorators
  • Added type: ignore comments for decorator return values
  • Removed string quotes from Credential return type annotations
+15/-10 

@qodo-code-review
Copy link
Contributor

qodo-code-review bot commented Jan 25, 2026

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

🔴
Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status: 🏷️
Broken equality logic: ScriptKey.eq compares the internal string _id directly to other (an object), causing
ScriptKey("x") == ScriptKey("x") to evaluate incorrectly and breaking
expected equality semantics.

Referred Code
def __eq__(self, other: object) -> bool:
    return self._id == other

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status: 🏷️
Unvalidated external values: New # type: ignore-guarded attribute chains (e.g., splitting se:cdpVersion and accessing
goog:chromeOptions/ms:edgeOptions) may still raise runtime errors or accept malformed
capability values without validation, but full correctness depends on guarantees outside
the diff.

Referred Code
global cdp
import_cdp()
if self.caps.get("se:cdp"):
    ws_url = self.caps.get("se:cdp")
    version = self.caps.get("se:cdpVersion").split(".")[0]  # type: ignore[union-attr]
else:

Learn more about managing compliance generic rules or creating your own custom rules

  • Update
Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-code-review
Copy link
Contributor

qodo-code-review bot commented Jan 25, 2026

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Add explicit checks for None values

Add explicit None checks in _get_cdp_details for debugger_address,

browser_version, and the regex match result to prevent potential runtime errors

and improve robustness.

py/selenium/webdriver/remote/webdriver.py [1317-1341]

 def _get_cdp_details(self) -> tuple[str, str]:
     import json
 
     import urllib3
 
     http = urllib3.PoolManager()
-    try:
-        if self.caps.get("browserName") == "chrome":
-            debugger_address = self.caps.get("goog:chromeOptions").get("debuggerAddress")  # type: ignore[union-attr]
-        elif self.caps.get("browserName") in ("MicrosoftEdge", "webview2"):
-            debugger_address = self.caps.get("ms:edgeOptions").get("debuggerAddress")  # type: ignore[union-attr]
-    except AttributeError:
+    debugger_address = None
+    if self.caps.get("browserName") == "chrome":
+        debugger_address = self.caps.get("goog:chromeOptions", {}).get("debuggerAddress")
+    elif self.caps.get("browserName") in ("MicrosoftEdge", "webview2"):
+        debugger_address = self.caps.get("ms:edgeOptions", {}).get("debuggerAddress")
+
+    if not debugger_address:
         raise WebDriverException("Can't get debugger address.")
 
     res = http.request("GET", f"http://{debugger_address}/json/version")
     data = json.loads(res.data)
 
     browser_version = data.get("Browser")
+    if not browser_version:
+        raise WebDriverException("Can't get browser version from CDP.")
+
     websocket_url = data.get("webSocketDebuggerUrl")
+    if not websocket_url:
+        raise WebDriverException("Can't get websocket URL from CDP.")
 
     import re
 
-    version = re.search(r".*/(\d+)\.", browser_version).group(1)  # type: ignore[union-attr]
+    match = re.search(r".*/(\d+)\.", browser_version)
+    if not match:
+        raise WebDriverException(f"Can't parse browser version: {browser_version}")
+    version = match.group(1)
 
     return version, websocket_url
  • Apply / Chat
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies multiple potential runtime errors from unchecked
None values, which are currently suppressed by type: ignore. Adding explicit
checks makes the code more robust and improves error handling.

Medium
Fix equality logic
Suggestion Impact:Updated ScriptKey.__eq__ to handle comparisons with ScriptKey instances and strings explicitly, and return NotImplemented for other types.

code diff:

     def __eq__(self, other: object) -> bool:
-        return self._id == other
+        if isinstance(other, ScriptKey):
+            return self._id == other._id
+        if isinstance(other, str):
+            return self._id == other
+        return NotImplemented

Fix the eq method in ScriptKey to correctly compare with other ScriptKey

instances and strings, and to return NotImplemented for unsupported types.

py/selenium/webdriver/remote/script_key.py [33-34]

 def __eq__(self, other: object) -> bool:
-    return self._id == other
+    if isinstance(other, ScriptKey):
+        return self._id == other._id
+    if isinstance(other, str):
+        return self._id == other
+    return NotImplemented

[Suggestion processed]

Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies a logic flaw in the __eq__ method and provides
a robust fix that handles comparisons with other ScriptKey instances, strings,
and other types by returning NotImplemented.

Medium
General
Remove obsolete response parsing logic

Remove the obsolete logic for handling a nested 'value' key in get_window_size,

as get_window_rect now returns an unwrapped dictionary.

py/selenium/webdriver/remote/webdriver.py [922-934]

 def get_window_size(self, windowHandle: str = "current") -> dict[str, int]:
     """Gets the width and height of the current window.
 
     Example:
         `driver.get_window_size()`
     """
     self._check_if_window_handle_is_current(windowHandle)
     size = self.get_window_rect()
 
-    if size.get("value", None):
-        size = size["value"]  # type: ignore[assignment]
-
     return {k: size[k] for k in ("width", "height")}
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies that due to changes in get_window_rect, the
logic to unwrap a value dictionary in get_window_size is now obsolete and
should be removed for consistency and clarity.

Low
Learned
best practice
Replace asserts with explicit guards

Replace the assert with explicit checks that browserName exists and is a string,
and raise a consistent exception (e.g., WebDriverException/ValueError) so the
guard works even under Python -O.

py/selenium/webdriver/common/virtual_authenticator.py [200-211]

 def required_chromium_based_browser(func: F) -> F:
     """Decorator to ensure that the client used is a chromium-based browser."""
 
     @functools.wraps(func)
     def wrapper(self: Any, *args: Any, **kwargs: Any) -> Any:
-        assert self.caps["browserName"].lower() not in [
-            "firefox",
-            "safari",
-        ], "This only currently works in Chromium based browsers"
+        browser_name = self.caps.get("browserName")
+        if not isinstance(browser_name, str) or not browser_name:
+            raise ValueError("browserName not specified in session capabilities")
+        if browser_name.lower() in ("firefox", "safari"):
+            raise ValueError("This only currently works in Chromium based browsers")
         return func(self, *args, **kwargs)
 
     return wrapper  # type: ignore[return-value]
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why:
Relevant best practice - Add explicit validation/guards at integration boundaries; avoid assert for runtime checks that must always execute.

Low
  • Update

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants