-
-
Notifications
You must be signed in to change notification settings - Fork 8.6k
.pr_agent_accepted_suggestions
| PR 16782 (2025-12-22) |
[possible issue] Remove ineffective window style setting
✅ Remove ineffective window style setting
Remove the ineffective WindowStyle assignment, as it has no effect when UseShellExecute is false. The existing CreateNoWindow property already handles hiding the window.
dotnet/src/webdriver/DriverService.cs [254-260]
this.driverServiceProcess.StartInfo.UseShellExecute = false;
this.driverServiceProcess.StartInfo.CreateNoWindow = this.HideCommandPromptWindow;
-if (this.HideCommandPromptWindow)
-{
- this.driverServiceProcess.StartInfo.WindowStyle = ProcessWindowStyle.Hidden;
-}
-Suggestion importance[1-10]: 7
__
Why: The suggestion correctly identifies that the code added in the PR is ineffective because UseShellExecute is set to false, and it proposes removing the dead code, which is the optimal fix.
| PR 16736 (2025-12-15) |
[possible issue] Prevent build failures from conflicting conditions
✅ Prevent build failures from conflicting conditions
To prevent build failures from conflicting conditions in the select statement, use selects.config_setting_group to create a mutually exclusive condition. This ensures that building from source only occurs when the OS matches and neither the use_pinned_browser nor stamp flags are set.
common/manager/BUILD.bazel [12-20]
+load("//bazel:rules.bzl", "selects")
+
+selects.config_setting_group(
+ name = "build_sm_linux_from_source",
+ match_all = [
+ "//common:linux",
+ ],
+ match_none = [
+ "//common:use_pinned_browser",
+ "//common:stamp",
+ ],
+)
+
alias(
name = "selenium-manager-linux",
actual = select({
"//common:use_pinned_browser": "@download_sm_linux//file",
"//common:stamp": "@download_sm_linux//file",
- "//common:linux": "//rust:selenium-manager-linux",
+ ":build_sm_linux_from_source": "//rust:selenium-manager-linux",
"//conditions:default": "@download_sm_linux//file",
}),
)Suggestion importance[1-10]: 9
__
Why: The suggestion correctly identifies a critical flaw in the select logic that would cause build failures when multiple independent conditions are met, and it provides the correct, idiomatic Bazel fix using config_setting_group to ensure mutual exclusivity.
| PR 16730 (2025-12-15) |
[possible issue] Fix incorrect __new__ method signature
✅ Fix incorrect new method signature
**kwargs from the object.new call (changing it to object.new(cls)), which prevents the TypeError the suggestion warned about. However, it did not switch to super().new(cls) as suggested.
code diff:
def __new__(cls, *args, **kwargs):
if cls is LocalWebDriver:
raise TypeError(f"Only children of '{cls.__name__}' may be instantiated")
- return object.__new__(cls, *args, **kwargs)
+ return object.__new__(cls)**In LocalWebDriver.new, replace the incorrect call to object.new(cls, *args, kwargs) with super().new(cls) to prevent a TypeError during instantiation.
py/selenium/webdriver/common/webdriver.py [29-32]
def __new__(cls, *args, **kwargs):
if cls is LocalWebDriver:
raise TypeError(f"Only children of '{cls.__name__}' may be instantiated")
- return object.__new__(cls, *args, **kwargs)
+ return super().__new__(cls)Suggestion importance[1-10]: 9
__
Why: The suggestion correctly identifies a critical bug in the __new__ method that would cause a TypeError when instantiating any subclass of LocalWebDriver, making the core refactoring of the PR non-functional.
| PR 16717 (2025-12-11) |
[possible issue] Add missing JSON type property
✅ Add missing JSON type property
Add the missing Type property to the RemoteReferenceLocalValue abstract record to ensure correct JSON serialization for its derived classes.
dotnet/src/webdriver/BiDi/Script/LocalValue.cs [270-280]
-public abstract record RemoteReferenceLocalValue : LocalValue, IRemoteReference;
+public abstract record RemoteReferenceLocalValue : LocalValue, IRemoteReference
+{
+ [JsonInclude]
+ internal string Type { get; } = "remote";
+}
public sealed record SharedReferenceLocalValue(string SharedId) : RemoteReferenceLocalValue, ISharedReference
{
public Handle? Handle { get; set; }
}
public sealed record RemoteObjectReferenceLocalValue(Handle Handle) : RemoteReferenceLocalValue, IRemoteObjectReference
{
public string? SharedId { get; set; }
}Suggestion importance[1-10]: 9
__
Why: The suggestion correctly identifies that the new RemoteReferenceLocalValue and its derived classes are missing the Type property, which is essential for correct JSON serialization and is inconsistent with the pattern applied to all other types in this PR.
[learned best practice] Centralize type discriminator logic
✅ Centralize type discriminator logic
Avoid repeating the discriminator property across all records; centralize it in a shared base/helper to prevent drift and reduce boilerplate.
dotnet/src/webdriver/BiDi/Script/LocalValue.cs [284-362]
-public sealed record NumberLocalValue([property: JsonConverter(typeof(SpecialNumberConverter))] double Value) : PrimitiveProtocolLocalValue
+public abstract record TypedLocalValue(string Discriminator) : LocalValue
{
[JsonInclude]
- internal string Type { get; } = "number";
+ internal string Type { get; } = Discriminator;
+}
+public abstract record TypedPrimitiveLocalValue(string Discriminator) : PrimitiveProtocolLocalValue
+{
+ [JsonInclude]
+ internal string Type { get; } = Discriminator;
+}
+
+public sealed record NumberLocalValue([property: JsonConverter(typeof(SpecialNumberConverter))] double Value)
+ : TypedPrimitiveLocalValue("number")
+{
public static explicit operator NumberLocalValue(double n) => new NumberLocalValue(n);
}
-public sealed record StringLocalValue(string Value) : PrimitiveProtocolLocalValue
-{
- [JsonInclude]
- internal string Type { get; } = "string";
-}
+public sealed record StringLocalValue(string Value) : TypedPrimitiveLocalValue("string");
+public sealed record NullLocalValue() : TypedPrimitiveLocalValue("null");
+public sealed record UndefinedLocalValue() : TypedPrimitiveLocalValue("undefined");
+public sealed record BooleanLocalValue(bool Value) : TypedPrimitiveLocalValue("boolean");
+public sealed record BigIntLocalValue(string Value) : TypedPrimitiveLocalValue("bigint");
-public sealed record NullLocalValue : PrimitiveProtocolLocalValue
-{
- [JsonInclude]
- internal string Type { get; } = "null";
-}
+public sealed record ChannelLocalValue(ChannelProperties Value) : TypedLocalValue("channel");
+public sealed record ArrayLocalValue(IEnumerable<LocalValue> Value) : TypedLocalValue("array");
+public sealed record DateLocalValue(string Value) : TypedLocalValue("date");
+public sealed record MapLocalValue(IEnumerable<IEnumerable<LocalValue>> Value) : TypedLocalValue("map");
+public sealed record ObjectLocalValue(IEnumerable<IEnumerable<LocalValue>> Value) : TypedLocalValue("object");
+public sealed record RegExpLocalValue(RegExpValue Value) : TypedLocalValue("regexp");
+public sealed record SetLocalValue(IEnumerable<LocalValue> Value) : TypedLocalValue("set");
-public sealed record UndefinedLocalValue : PrimitiveProtocolLocalValue
-{
- [JsonInclude]
- internal string Type { get; } = "undefined";
-}
-
-public sealed record BooleanLocalValue(bool Value) : PrimitiveProtocolLocalValue
-{
- [JsonInclude]
- internal string Type { get; } = "boolean";
-}
-
-public sealed record BigIntLocalValue(string Value) : PrimitiveProtocolLocalValue
-{
- [JsonInclude]
- internal string Type { get; } = "bigint";
-}
-
-public sealed record ChannelLocalValue(ChannelProperties Value) : LocalValue
-{
- [JsonInclude]
- internal string Type { get; } = "channel";
-}
-
-public sealed record ArrayLocalValue(IEnumerable<LocalValue> Value) : LocalValue
-{
- [JsonInclude]
- internal string Type { get; } = "array";
-}
-
-public sealed record DateLocalValue(string Value) : LocalValue
-{
- [JsonInclude]
- internal string Type { get; } = "date";
-}
-
-public sealed record MapLocalValue(IEnumerable<IEnumerable<LocalValue>> Value) : LocalValue
-{
- [JsonInclude]
- internal string Type { get; } = "map";
-}
-
-public sealed record ObjectLocalValue(IEnumerable<IEnumerable<LocalValue>> Value) : LocalValue
-{
- [JsonInclude]
- internal string Type { get; } = "object";
-}
-
-public sealed record RegExpLocalValue(RegExpValue Value) : LocalValue
-{
- [JsonInclude]
- internal string Type { get; } = "regexp";
-}
-
-public sealed record SetLocalValue(IEnumerable<LocalValue> Value) : LocalValue
-{
- [JsonInclude]
- internal string Type { get; } = "set";
-}
-Suggestion importance[1-10]: 6
__
Why: Relevant best practice - Replace ad-hoc duplication with shared helpers/utilities to centralize logic and reduce repetition.
| PR 16711 (2025-12-09) |
[learned best practice] Validate environment variable path
✅ Validate environment variable path
Validate that SE_MANAGER_PATH points to an existing executable file and ignore or error out if it does not.
py/selenium/webdriver/common/selenium_manager.py [81-83]
if (env_path := os.getenv("SE_MANAGER_PATH")) is not None:
logger.debug(f"Selenium Manager set by env SE_MANAGER_PATH to: {env_path}")
- path = Path(env_path)
+ env_path = env_path.strip()
+ candidate = Path(env_path)
+ if candidate.is_file():
+ path = candidate
+ else:
+ logger.warning(f"SE_MANAGER_PATH does not point to a file: {env_path}")Suggestion importance[1-10]: 6
__
Why: Relevant best practice - Validate and sanitize external inputs such as environment variables before use to avoid invalid paths or injection issues.
| PR 16705 (2025-12-08) |
[learned best practice] Return unmodifiable serialized map
✅ Return unmodifiable serialized map
Return an unmodifiable copy to prevent callers from mutating the returned map; this preserves immutability of serialized state.
java/src/org/openqa/selenium/bidi/emulation/ScreenOrientation.java [46-51]
public Map<String, Object> toMap() {
Map<String, Object> map = new HashMap<>();
map.put("natural", natural.toString());
map.put("type", type.toString());
- return map;
+ return java.util.Collections.unmodifiableMap(map);
}Suggestion importance[1-10]: 6
__
Why: Relevant best practice - Use defensive copying and immutability for maps returned by accessors to avoid external mutation.
[general] Remove inconsistent and unnecessary page reload
✅ Remove inconsistent and unnecessary page reload
Remove the inconsistent and unnecessary page reload after the first setScreenOrientationOverride call to make the test accurately reflect the expected dynamic behavior.
java/test/org/openqa/selenium/bidi/emulation/SetScreenOrientationOverrideTest.java [68-93]
-// Reload the page to apply the orientation change
-context.navigate(url, ReadinessState.COMPLETE);
-
Map<String, Object> currentOrientation = getScreenOrientation(contextId);
assertThat(currentOrientation.get("type")).isEqualTo("landscape-primary");
assertThat(currentOrientation.get("angle")).isEqualTo(0);
// Set portrait-secondary orientation
ScreenOrientation portraitOrientation =
new ScreenOrientation(
ScreenOrientationNatural.PORTRAIT, ScreenOrientationType.PORTRAIT_SECONDARY);
emulation.setScreenOrientationOverride(
new SetScreenOrientationOverrideParameters(portraitOrientation)
.contexts(List.of(contextId)));
currentOrientation = getScreenOrientation(contextId);
assertThat(currentOrientation.get("type")).isEqualTo("portrait-secondary");
assertThat(currentOrientation.get("angle")).isEqualTo(180);
// Clear the override
emulation.setScreenOrientationOverride(
new SetScreenOrientationOverrideParameters(null).contexts(List.of(contextId)));
currentOrientation = getScreenOrientation(contextId);
assertThat(currentOrientation.get("type")).isEqualTo(initialOrientation.get("type"));
assertThat(currentOrientation.get("angle")).isEqualTo(initialOrientation.get("angle"));Suggestion importance[1-10]: 7
__
Why: The suggestion correctly identifies an inconsistent and likely unnecessary page reload in the test, improving test correctness and consistency.
[general] Use idiomatic null checks for consistency
✅ Use idiomatic null checks for consistency
In the ScreenOrientation constructor, replace manual null checks with the idiomatic Require.nonNull() for consistency with the rest of the codebase.
java/src/org/openqa/selenium/bidi/emulation/ScreenOrientation.java [27-36]
-public ScreenOrientation(ScreenOrientationNatural natural, ScreenOrientationType type) {
- if (natural == null) {
- throw new IllegalArgumentException("Natural orientation cannot be null");
+import org.openqa.selenium.internal.Require;
+
+public class ScreenOrientation {
+ // ...
+ public ScreenOrientation(ScreenOrientationNatural natural, ScreenOrientationType type) {
+ this.natural = Require.nonNull("Natural orientation", natural);
+ this.type = Require.nonNull("Orientation type", type);
}
- if (type == null) {
- throw new IllegalArgumentException("Orientation type cannot be null");
- }
- this.natural = natural;
- this.type = type;
+ // ...
}Suggestion importance[1-10]: 4
__
Why: The suggestion improves code consistency by proposing the use of the idiomatic Require.nonNull() helper, aligning the new class with existing codebase conventions.
[possible issue] Fetch screen orientation properties atomically
✅ Fetch screen orientation properties atomically
Refactor the getScreenOrientation method to fetch screen orientation type and angle in a single, atomic JavaScript execution to prevent potential race conditions.
java/test/org/openqa/selenium/bidi/emulation/SetScreenOrientationOverrideTest.java [36-44]
private Map<String, Object> getScreenOrientation(String context) {
driver.switchTo().window(context);
JavascriptExecutor executor = (JavascriptExecutor) driver;
- String type = (String) executor.executeScript("return screen.orientation.type;");
- Number angle = (Number) executor.executeScript("return screen.orientation.angle;");
+ Map<String, Object> orientation =
+ (Map<String, Object>)
+ executor.executeScript(
+ "return { type: screen.orientation.type, angle: screen.orientation.angle };");
- return Map.of("type", type, "angle", angle.intValue());
+ return Map.of("type", orientation.get("type"), "angle", ((Number) orientation.get("angle")).intValue());
}Suggestion importance[1-10]: 6
__
Why: The suggestion correctly identifies a potential race condition in a test helper method and proposes a more robust, atomic operation which also slightly improves performance.
| PR 16678 (2025-12-04) |
[high-level] Avoid modifying the core SessionId class
✅ Avoid modifying the core SessionId class
**
-
- Sets the reason why this session was closed. Once set, indicates the session is no longer
-
- active.
-
-
- @param reason The reason for session closure
- */
- public void setCloseReason(String reason) {
- this.closeReason = reason;
- }
- /**
-
- Gets the reason why this session was closed, if any.
-
-
- @return The close reason, or null if the session is still active
- */
- public @Nullable String getCloseReason() {
- return closeReason;
- }
- /**
-
- Checks if this session has been closed.
-
-
- @return true if the session has a close reason set, false otherwise
- */
- public boolean isClosed() {
- return closeReason != null; }
@Override @@ -85,17 +53,7 @@ }
private Object toJson() {
- // For backward compatibility, serialize as string when there's no closeReason
- // This ensures SessionId works properly in URLs and simple contexts
- if (closeReason == null) {
-
return opaqueKey; - }
- // When there is a closeReason, serialize as Map to preserve the metadata
- Map<String, Object> json = new HashMap<>();
- json.put("value", opaqueKey);
- json.put("closeReason", closeReason);
- return json;
- return opaqueKey; }
private static SessionId fromJson(Object raw) { @@ -103,18 +61,6 @@ return new SessionId(String.valueOf(raw)); }
- if (raw instanceof Map) {
-
Map<?, ?> map = (Map<?, ?>) raw; -
if (map.get("value") instanceof String) { -
SessionId sessionId = new SessionId(String.valueOf(map.get("value"))); -
// Restore closeReason if present -
if (map.get("closeReason") instanceof String) { -
sessionId.setCloseReason(String.valueOf(map.get("closeReason"))); -
} -
return sessionId; -
} - }
- throw new JsonException("Unable to coerce session id from " + raw); } }
@@ -34,21 +34,23 @@ }
public SessionClosedEvent(SessionId id, SessionClosedReason reason) {
- super(SESSION_CLOSED, markSessionId(id, reason));
- super(SESSION_CLOSED, new SessionClosedData(id, reason)); Require.nonNull("Session ID", id); Require.nonNull("Reason", reason); }
- // Helper method to mark the SessionId before passing to Event constructor
- private static SessionId markSessionId(SessionId id, SessionClosedReason reason) {
- id.setCloseReason(reason.getReasonText());
- return id;
- // Standard listener method that provides access to both SessionId and reason
- public static EventListener listener(Consumer handler) {
- Require.nonNull("Handler", handler);
- return new EventListener<>(SESSION_CLOSED, SessionClosedData.class, handler); }
- // Standard listener method following the Event pattern
- public static EventListener listener(Consumer handler) {
- // Convenience method for listeners that only care about the SessionId
- public static EventListener sessionListener(Consumer handler) { Require.nonNull("Handler", handler);
- return new EventListener<>(SESSION_CLOSED, SessionId.class, handler);
- return new EventListener<>(
-
} }
SESSION_CLOSED, SessionClosedData.class, data -> handler.accept(data.getSessionId()));
**Avoid modifying the core SessionId class to include session closure state and changing its JSON format. Instead, manage the closure reason separately to prevent a significant breaking change.**
### Examples:
java/src/org/openqa/selenium/remote/SessionId.java [33-115]
```java
private @Nullable String closeReason;
public SessionId(UUID uuid) {
this(Require.nonNull("Session ID key", uuid).toString());
}
public SessionId(String opaqueKey) {
this.opaqueKey = Require.nonNull("Session ID key", opaqueKey);
this.closeReason = null; // Session is alive initially
}
... (clipped 73 lines)
java/src/org/openqa/selenium/grid/data/SessionClosedEvent.java [36-46]
public SessionClosedEvent(SessionId id, SessionClosedReason reason) {
super(SESSION_CLOSED, markSessionId(id, reason));
Require.nonNull("Session ID", id);
Require.nonNull("Reason", reason);
}
// Helper method to mark the SessionId before passing to Event constructor
private static SessionId markSessionId(SessionId id, SessionClosedReason reason) {
id.setCloseReason(reason.getReasonText());
return id;
... (clipped 1 lines)// In SessionId.java
class SessionId {
private final String opaqueKey;
private @Nullable String closeReason;
public void setCloseReason(String reason) { ... }
private Object toJson() {
if (closeReason == null) {
return opaqueKey; // Returns a string
}
// Returns a map, which is a breaking change
return Map.of("value", opaqueKey, "closeReason", closeReason);
}
}
// In SessionClosedEvent.java
class SessionClosedEvent extends Event {
public SessionClosedEvent(SessionId id, SessionClosedReason reason) {
super(SESSION_CLOSED, markSessionId(id, reason)); // Modifies SessionId state
}
}// In SessionId.java (reverted to original state)
class SessionId {
private final String opaqueKey;
private String toJson() {
return opaqueKey; // Always returns a string
}
}
// In a new file, e.g., SessionClosure.java
class SessionClosure {
private final SessionId sessionId;
private final SessionClosedReason reason;
}
// In SessionClosedEvent.java
class SessionClosedEvent extends Event {
public SessionClosedEvent(SessionId id, SessionClosedReason reason) {
super(SESSION_CLOSED, new SessionClosure(id, reason)); // Pass a dedicated object
}
}Suggestion importance[1-10]: 9
__
Why: This suggestion correctly identifies a significant, potentially breaking change to the core SessionId class and its JSON serialization, proposing a safer, non-breaking alternative that achieves the same goal.
[learned best practice] Make SessionId immutable
✅ Make SessionId immutable
**
-
- Sets the reason why this session was closed. Once set, indicates the session is no longer
-
- active.
-
-
- @param reason The reason for session closure
- */
- public void setCloseReason(String reason) {
- this.closeReason = reason;
- }
- /**
-
- Gets the reason why this session was closed, if any.
-
-
- @return The close reason, or null if the session is still active
- */
- public @Nullable String getCloseReason() {
- return closeReason;
- }
- /**
-
- Checks if this session has been closed.
-
-
- @return true if the session has a close reason set, false otherwise
- */
- public boolean isClosed() {
- return closeReason != null; }
@Override @@ -85,17 +53,7 @@ }
private Object toJson() {
- // For backward compatibility, serialize as string when there's no closeReason
- // This ensures SessionId works properly in URLs and simple contexts
- if (closeReason == null) {
-
return opaqueKey; - }
- // When there is a closeReason, serialize as Map to preserve the metadata
- Map<String, Object> json = new HashMap<>();
- json.put("value", opaqueKey);
- json.put("closeReason", closeReason);
- return json;
- return opaqueKey; }
private static SessionId fromJson(Object raw) { @@ -103,18 +61,6 @@ return new SessionId(String.valueOf(raw)); }
- if (raw instanceof Map) {
-
Map<?, ?> map = (Map<?, ?>) raw; -
if (map.get("value") instanceof String) { -
SessionId sessionId = new SessionId(String.valueOf(map.get("value"))); -
// Restore closeReason if present -
if (map.get("closeReason") instanceof String) { -
sessionId.setCloseReason(String.valueOf(map.get("closeReason"))); -
} -
return sessionId; -
} - }
- throw new JsonException("Unable to coerce session id from " + raw); }
**Avoid making SessionId mutable and externally settable; carry close reason separately or return a defensive copy during serialization to prevent unintended mutation and cross-component coupling.**
[java/src/org/openqa/selenium/remote/SessionId.java [30-120]](https://github.com/SeleniumHQ/selenium/pull/16678/files#diff-c5b09ea9aa7ec16a444542ec011f3305f89d73aee6239e9cb1f5f314a7426ca2R30-R120)
```diff
-public class SessionId implements Serializable {
+public final class SessionId implements Serializable {
private final String opaqueKey;
- private @Nullable String closeReason;
-...
- public void setCloseReason(String reason) {
- this.closeReason = reason;
+ private final @Nullable String closeReason;
+
+ public SessionId(UUID uuid) {
+ this(Require.nonNull("Session ID key", uuid).toString(), null);
}
-...
+
+ public SessionId(String opaqueKey) {
+ this(opaqueKey, null);
+ }
+
+ private SessionId(String opaqueKey, @Nullable String closeReason) {
+ this.opaqueKey = Require.nonNull("Session ID key", opaqueKey);
+ this.closeReason = closeReason;
+ }
+
+ public SessionId withCloseReason(String reason) {
+ return new SessionId(this.opaqueKey, reason);
+ }
+
+ public @Nullable String getCloseReason() {
+ return closeReason;
+ }
+
+ public boolean isClosed() {
+ return closeReason != null;
+ }
+
private Object toJson() {
- // For backward compatibility, serialize as string when there's no closeReason
- // This ensures SessionId works properly in URLs and simple contexts
if (closeReason == null) {
return opaqueKey;
}
-
- // When there is a closeReason, serialize as Map to preserve the metadata
Map<String, Object> json = new HashMap<>();
json.put("value", opaqueKey);
json.put("closeReason", closeReason);
return json;
}
+ private static SessionId fromJson(Object raw) {
+ if (raw instanceof String) {
+ return new SessionId(String.valueOf(raw));
+ }
+ if (raw instanceof Map) {
+ Map<?, ?> map = (Map<?, ?>) raw;
+ if (map.get("value") instanceof String) {
+ String value = String.valueOf(map.get("value"));
+ String reason = map.get("closeReason") instanceof String ? String.valueOf(map.get("closeReason")) : null;
+ return new SessionId(value, reason);
+ }
+ }
+ throw new JsonException("Unable to coerce session id from " + raw);
+ }
+}
+
Suggestion importance[1-10]: 6
__
Why: Relevant best practice - Use defensive copying and avoid exposing or mutating shared internal state through objects passed across system boundaries.
[learned best practice] Remove side-effectful mutation
✅ Remove side-effectful mutation
Avoid mutating SessionId in a helper; instead create a new value or event payload carrying the reason to prevent hidden side effects.
java/src/org/openqa/selenium/grid/data/SessionClosedEvent.java [36-46]
public SessionClosedEvent(SessionId id, SessionClosedReason reason) {
- super(SESSION_CLOSED, markSessionId(id, reason));
Require.nonNull("Session ID", id);
Require.nonNull("Reason", reason);
+ super(SESSION_CLOSED, id.withCloseReason(reason.getReasonText()));
}
-// Helper method to mark the SessionId before passing to Event constructor
-private static SessionId markSessionId(SessionId id, SessionClosedReason reason) {
- id.setCloseReason(reason.getReasonText());
- return id;
-}
-Suggestion importance[1-10]: 5
__
Why: Relevant best practice - Replace ad-hoc duplication with shared helpers/utilities and avoid side effects in helper methods.
[general] Return an immutable set copy
✅ Return an immutable set copy
In getSessionsByUri, replace new HashSet<>(result) with Set.copyOf(result) to return an immutable copy of the set, which is a safer programming practice.
java/src/org/openqa/selenium/grid/sessionmap/local/LocalSessionMap.java [293-297]
public Set<SessionId> getSessionsByUri(URI uri) {
Set<SessionId> result = sessionsByUri.get(uri);
// Return a copy to prevent concurrent modification issues
- return (result != null && !result.isEmpty()) ? new HashSet<>(result) : Set.of();
+ return (result != null && !result.isEmpty()) ? Set.copyOf(result) : Set.of();
}Suggestion importance[1-10]: 5
__
Why: This is a good suggestion that improves code robustness by returning an immutable collection, which prevents callers from making unintended modifications and aligns with best practices for encapsulation.
| PR 16670 (2025-12-03) |
[possible issue] Hydrate event arguments after deserialization
✅ Hydrate event arguments after deserialization
Add hydration logic for deserialized event arguments in the ProcessReceivedMessage method, similar to how command results are handled, to prevent runtime errors.
dotnet/src/webdriver/BiDi/Broker.cs [282-285]
+if (commandResult is IBiDiHydratable bidiHydratable)
+{
+ bidiHydratable.Hydrate(_bidi);
+}
-Suggestion importance[1-10]: 9
__
Why: The suggestion correctly identifies a critical bug where event arguments are not hydrated, which would lead to NullReferenceException at runtime, defeating a major goal of the PR.
[learned best practice] Eliminate dead JSON options
✅ Eliminate dead JSON options
Remove the now-unused JsonSerializerOptions method and inline config; prefer the new source-generated contexts to avoid dead code and duplication.
dotnet/src/webdriver/BiDi/BiDi.cs [95-117]
-return new JsonSerializerOptions
-{
- //PropertyNameCaseInsensitive = true,
- //PropertyNamingPolicy = JsonNamingPolicy.CamelCase,
- //DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull,
+// Remove GetJsonOptions() entirely and references to it.
+// Modules now use source-generated contexts (e.g., BrowsingContextJsonSerializerContext.Default)
- // BiDi returns special numbers such as "NaN" as strings
- // Additionally, -0 is returned as a string "-0"
- NumberHandling = JsonNumberHandling.AllowNamedFloatingPointLiterals | JsonNumberHandling.AllowReadingFromString,
- Converters =
- {
- //new BrowsingContextConverter(),
- //new BrowserUserContextConverter(),
- //new CollectorConverter(),
- //new InterceptConverter(),
- //new HandleConverter(),
- //new InternalIdConverter(),
- //new PreloadScriptConverter(),
- //new RealmConverter(),
- new DateTimeOffsetConverter(),
- //new WebExtensionConverter(),
- }
-};
-Suggestion importance[1-10]: 6
__
Why: Relevant best practice - Pattern 4: Centralize shared configuration logic to avoid duplication and ensure consistency (e.g., JSON options/converters).
[high-level] Add runtime checks for hydration
✅ Add runtime checks for hydration
To prevent NullReferenceExceptions, add a runtime check to the BiDi property getter on objects using the new hydration pattern. This check should throw an InvalidOperationException if the property is accessed before it has been hydrated.
dotnet/src/webdriver/BiDi/Browser/UserContext.cs [38]
public BiDi BiDi { get; internal set; }dotnet/src/webdriver/BiDi/BrowsingContext/BrowsingContext.cs [45]
public BiDi BiDi { get; internal set; }// In dotnet/src/webdriver/BiDi/Browser/UserContext.cs
public sealed class UserContext : IAsyncDisposable
{
// ... constructor ...
[JsonIgnore]
public BiDi BiDi { get; internal set; }
public Task RemoveAsync()
{
// This will throw NullReferenceException if BiDi is not hydrated
return BiDi.Browser.RemoveUserContextAsync(this);
}
// ...
}// In dotnet/src/webdriver/BiDi/Browser/UserContext.cs
public sealed class UserContext : IAsyncDisposable
{
private BiDi? _bidi;
// ... constructor ...
[JsonIgnore]
public BiDi BiDi
{
get => _bidi ?? throw new InvalidOperationException("BiDi instance has not been hydrated.");
internal set => _bidi = value;
}
public Task RemoveAsync()
{
return BiDi.Browser.RemoveUserContextAsync(this);
}
// ...
}Suggestion importance[1-10]: 8
__
Why: The suggestion correctly identifies a significant design weakness in the new hydration pattern, preventing potential NullReferenceExceptions and improving API robustness with explicit, fail-fast checks.
| PR 16664 (2025-12-01) |
[possible issue] Fix race condition with thread-safe initialization
✅ Fix race condition with thread-safe initialization
The lazy initialization using ??= is not thread-safe and can cause a race condition. To fix this, use Interlocked.CompareExchange for a lock-free, thread-safe initialization and mark the backing fields as volatile.
dotnet/src/webdriver/BiDi/BrowsingContext/BrowsingContext.cs [34-58]
-private BrowsingContextLogModule? _logModule;
-private BrowsingContextNetworkModule? _networkModule;
-private BrowsingContextScriptModule? _scriptModule;
-private BrowsingContextStorageModule? _storageModule;
-private BrowsingContextInputModule? _inputModule;
+private volatile BrowsingContextLogModule? _logModule;
+private volatile BrowsingContextNetworkModule? _networkModule;
+private volatile BrowsingContextScriptModule? _scriptModule;
+private volatile BrowsingContextStorageModule? _storageModule;
+private volatile BrowsingContextInputModule? _inputModule;
internal string Id { get; }
[JsonIgnore]
public BiDi BiDi { get; }
[JsonIgnore]
-public BrowsingContextLogModule Log => _logModule ??= new BrowsingContextLogModule(this, BiDi.Log);
+public BrowsingContextLogModule Log => _logModule ?? Interlocked.CompareExchange(ref _logModule, new BrowsingContextLogModule(this, BiDi.Log), null) ?? _logModule!;
[JsonIgnore]
-public BrowsingContextNetworkModule Network => _networkModule ??= new BrowsingContextNetworkModule(this, BiDi.Network);
+public BrowsingContextNetworkModule Network => _networkModule ?? Interlocked.CompareExchange(ref _networkModule, new BrowsingContextNetworkModule(this, BiDi.Network), null) ?? _networkModule!;
[JsonIgnore]
-public BrowsingContextScriptModule Script => _scriptModule ??= new BrowsingContextScriptModule(this, BiDi.Script);
+public BrowsingContextScriptModule Script => _scriptModule ?? Interlocked.CompareExchange(ref _scriptModule, new BrowsingContextScriptModule(this, BiDi.Script), null) ?? _scriptModule!;
[JsonIgnore]
-public BrowsingContextStorageModule Storage => _storageModule ??= new BrowsingContextStorageModule(this, BiDi.Storage);
+public BrowsingContextStorageModule Storage => _storageModule ?? Interlocked.CompareExchange(ref _storageModule, new BrowsingContextStorageModule(this, BiDi.Storage), null) ?? _storageModule!;
[JsonIgnore]
-public BrowsingContextInputModule Input => _inputModule ??= new BrowsingContextInputModule(this, BiDi.InputModule);
+public BrowsingContextInputModule Input => _inputModule ?? Interlocked.CompareExchange(ref _inputModule, new BrowsingContextInputModule(this, BiDi.InputModule), null) ?? _inputModule!;Suggestion importance[1-10]: 9
__
Why: The suggestion correctly identifies that the refactoring from Lazy to ??= introduces a race condition, as ??= is not an atomic operation. This is a critical regression in thread safety, and the proposed fix using Interlocked.CompareExchange is a correct and robust pattern for thread-safe lazy initialization.
| PR 16658 (2025-11-30) |
[general] Remove statement with no effect
✅ Remove statement with no effect
In the generate_from_json method, remove the line expr which is a statement with no effect and likely a debugging leftover.
def generate_from_json(self, dict_):
"""Generate the code that creates an instance from a JSON dict named `dict_`."""
if self.items:
if self.items.ref:
py_ref = ref_to_python(self.items.ref)
expr = f"[{py_ref}.from_json(i) for i in {dict_}['{self.name}']]"
- expr
else:
cons = CdpPrimitiveType.get_constructor(self.items.type, "i")
expr = f"[{cons} for i in {dict_}['{self.name}']]"
else:
if self.ref:
py_ref = ref_to_python(self.ref)
expr = f"{py_ref}.from_json({dict_}['{self.name}'])"
else:
expr = CdpPrimitiveType.get_constructor(self.type, f"{dict_}['{self.name}']")
if self.optional:
expr = f"{expr} if '{self.name}' in {dict_} else None"
return exprSuggestion importance[1-10]: 3
__
Why: The suggestion correctly identifies a useless statement on line 300 that has no effect on the program's logic, and removing it improves code clarity.
| PR 16650 (2025-11-29) |
[possible issue] Make the Dispose method idempotent
✅ Make the Dispose method idempotent
Make the Dispose method idempotent by using a private flag. This prevents returning the same buffer to the ArrayPool multiple times, which would otherwise cause a critical error.
dotnet/src/webdriver/BiDi/WebSocketTransport.cs [91-97]
+private bool _disposed;
+
public void Dispose()
{
+ if (_disposed)
+ {
+ return;
+ }
+
_webSocket.Dispose();
_sharedMemoryStream.Dispose();
_socketSendSemaphoreSlim.Dispose();
ArrayPool<byte>.Shared.Return(_receiveBuffer);
+ _disposed = true;
}Suggestion importance[1-10]: 9
__
Why: The suggestion correctly identifies a critical issue where calling Dispose multiple times would corrupt the ArrayPool by returning the same buffer more than once, potentially leading to memory corruption.
| PR 16647 (2025-11-27) |
[general] Improve window handle matching logic
✅ Improve window handle matching logic
In findTarget, change the filter logic from windowHandle.contains(id.toString()) to windowHandle.endsWith(id.toString()) for a more precise and robust matching of the window handle to the target ID.
java/src/org/openqa/selenium/devtools/DevTools.java [214-228]
private TargetID findTarget(String windowHandle) {
// Figure out the targets.
List<TargetInfo> infos =
connection.sendAndWait(cdpSession, getDomains().target().getTargets(), timeout);
// Grab the first "page" type, and glom on to that.
// Find out which one might be the current one
// (using given window handle like "CDwindow-24426957AC62D8BC83E58C184C38AF2D")
return infos.stream()
.filter(info -> "page".equals(info.getType()))
.map(TargetInfo::getTargetId)
- .filter(id -> windowHandle == null || windowHandle.contains(id.toString()))
+ .filter(id -> windowHandle == null || windowHandle.endsWith(id.toString()))
.findAny()
.orElseThrow(() -> new DevToolsException("Unable to find target id of a page"));
}Suggestion importance[1-10]: 4
__
Why: The suggestion correctly points out that using endsWith is more precise than contains for matching a windowHandle to a TargetID, making the implementation more robust against potential edge cases.
| PR 16646 (2025-11-27) |
[possible issue] Clean up temporary test directories
✅ Clean up temporary test directories
Add cleanup logic to the finally block to delete the temporary directory created during the test. This prevents resource leaks on the file system.
java/test/org/openqa/selenium/bidi/browser/BrowserCommandsTest.java [113-142]
Path tmpDir = TemporaryFilesystem.getDefaultTmpFS().createTempDir("downloads", "test").toPath();
try {
browser.setDownloadBehavior(new SetDownloadBehaviorParameters(true, tmpDir));
BrowsingContext context = new BrowsingContext(driver, driver.getWindowHandle());
String url = appServer.whereIs("downloads/download.html");
context.navigate(url, ReadinessState.COMPLETE);
driver.findElement(By.id("file-1")).click();
new WebDriverWait(driver, Duration.ofSeconds(5))
.until(
d -> {
try {
return Files.list(tmpDir)
.anyMatch(path -> path.getFileName().toString().equals("file_1.txt"));
} catch (Exception e) {
return false;
}
});
List<String> fileNames =
Files.list(tmpDir)
.map(path -> path.getFileName().toString())
.collect(Collectors.toList());
assertThat(fileNames).contains("file_1.txt");
} finally {
browser.setDownloadBehavior(new SetDownloadBehaviorParameters(null, (Path) null));
+ TemporaryFilesystem.getDefaultTmpFS().deleteTempDir(tmpDir.toFile());
}Suggestion importance[1-10]: 7
__
Why: The suggestion correctly identifies a resource leak where temporary directories created for tests are not cleaned up, which is important for test suite hygiene, especially on CI servers.
| PR 16631 (2025-11-24) |
[learned best practice] Cleanup created user contexts
✅ Cleanup created user contexts
Ensure created user contexts are cleaned up after the test by deleting them in a finally block to prevent resource leaks.
java/test/org/openqa/selenium/bidi/emulation/SetScriptingEnabledTest.java [82-113]
String userContext = browser.createUserContext();
-BrowsingContext context =
- new BrowsingContext(
- driver, new CreateContextParameters(WindowType.TAB).userContext(userContext));
-String contextId = context.getId();
-...
-// Clear the scripting override
-emulation.setScriptingEnabled(
- new SetScriptingEnabledParameters(null).userContexts(List.of(userContext)));
+try {
+ BrowsingContext context =
+ new BrowsingContext(
+ driver, new CreateContextParameters(WindowType.TAB).userContext(userContext));
+ String contextId = context.getId();
+ ...
+ emulation.setScriptingEnabled(
+ new SetScriptingEnabledParameters(null).userContexts(List.of(userContext)));
+} finally {
+ browser.deleteUserContext(userContext);
+}Suggestion importance[1-10]: 6
__
Why: Relevant best practice - Ensure test setup/teardown and resource cleanup to avoid leaking browser contexts or user contexts.
[general] Extract duplicated code into a helper
✅ Extract duplicated code into a helper
Extract the duplicated script evaluation logic for checking if 'foo' in window into a private helper method to reduce code repetition and improve test readability.
java/test/org/openqa/selenium/bidi/emulation/SetScriptingEnabledTest.java [57-74]
-EvaluateResult resultDisabled =
- script.evaluateFunctionInBrowsingContext(
- contextId, "'foo' in window", false, Optional.empty());
-Boolean valueDisabled =
- (Boolean) ((EvaluateResultSuccess) resultDisabled).getResult().getValue().get();
-assertThat(valueDisabled).isFalse();
+assertThat(isFooInWindow(contextId, script)).isFalse();
emulation.setScriptingEnabled(
new SetScriptingEnabledParameters(null).contexts(List.of(contextId)));
context.navigate(appServer.whereIs("script_page.html"), ReadinessState.COMPLETE);
-EvaluateResult resultEnabled =
- script.evaluateFunctionInBrowsingContext(
- contextId, "'foo' in window", false, Optional.empty());
-Boolean valueEnabled =
- (Boolean) ((EvaluateResultSuccess) resultEnabled).getResult().getValue().get();
-assertThat(valueEnabled).isTrue();
+assertThat(isFooInWindow(contextId, script)).isTrue();Suggestion importance[1-10]: 5
__
Why: The suggestion correctly identifies duplicated code in the test and proposes extracting it into a helper method, which improves code readability and maintainability.
[general] Simplify constructor logic for clarity
✅ Simplify constructor logic for clarity
Simplify the constructor logic by first checking for the invalid true case for the enabled parameter, then using a single map.put call for both null and false values.
java/src/org/openqa/selenium/bidi/emulation/SetScriptingEnabledParameters.java [22-30]
-// allow null
-if (enabled == null) {
- map.put("enabled", null);
-} else if (!enabled) {
- map.put("enabled", enabled);
-} else {
+if (enabled != null && enabled) {
throw new IllegalArgumentException(
"Only emulation of disabled JavaScript is supported (enabled must be false or null)");
}
+map.put("enabled", enabled);Suggestion importance[1-10]: 4
__
Why: The suggestion correctly simplifies the constructor's conditional logic, making the code more concise and readable without changing its behavior.
| PR 16628 (2025-11-22) |
[learned best practice] Pin dependency version
✅ Pin dependency version
Pin the jspecify artifact to a specific version to avoid accidental upgrades and non-reproducible builds.
java/src/org/openqa/selenium/chromium/BUILD.bazel [23]
-artifact("org.jspecify:jspecify"),
+artifact("org.jspecify:jspecify:1.0.0"),Suggestion importance[1-10]: 6
__
Why: Relevant best practice - Guard external dependencies with explicit versioning to ensure reproducible builds.
[learned best practice] Document nullable fields
✅ Document nullable fields
** May be null when the driver does not support casting; initialized during setup if available. */ protected @Nullable HasCasting casting;
- /** May be null when CDP is unavailable for the current browser/session. */ protected @Nullable HasCdp cdp;
**Document that casting and cdp may be null and describe when they are initialized or remain absent to align with the new annotations.**
[java/src/org/openqa/selenium/chromium/ChromiumDriver.java [97-98]](https://github.com/SeleniumHQ/selenium/pull/16628/files#diff-1af793ae2090a0cb691f1efbb3ee0d2b186c4027c5b441f6f3d55b3663e67ba1R97-R98)
```diff
+/** May be null when the driver does not support casting; initialized during setup if available. */
protected @Nullable HasCasting casting;
+/** May be null when CDP is unavailable for the current browser/session. */
protected @Nullable HasCdp cdp;
Suggestion importance[1-10]: 5
__
Why: Relevant best practice - Enforce accurate and consistent documentation and naming to reflect nullability behavior.
| PR 16627 (2025-11-22) |
[possible issue] Safely decode non-text and error responses
✅ Safely decode non-text and error responses
Refactor W3CHttpResponseCodec.decode to correctly handle binary content by keeping it as a stream supplier only when appropriate, and improve error handling for non-textual error responses to prevent crashes.
java/src/org/openqa/selenium/remote/codec/w3c/W3CHttpResponseCodec.java [75-175]
@Override
public Response decode(HttpResponse encodedResponse) {
if (LOG.isLoggable(Level.FINER)) {
LOG.log(
Level.FINER,
"Decoding response. Response code was: {0} and content: {1}",
- new Object[] {encodedResponse.getStatus(), encodedResponse.getContent()});
+ new Object[] {encodedResponse.getStatus(), encodedResponse.getHeader(HttpHeader.ContentType.getName())});
}
String contentType =
Objects.requireNonNullElse(encodedResponse.getHeader(HttpHeader.ContentType.getName()), "");
Response response = new Response();
- // Are we dealing with an error?
- // {"error":"no such alert","message":"No tab modal was open when attempting to get the dialog
- // text"}
if (!encodedResponse.isSuccessful()) {
LOG.fine("Processing an error");
- String content = encodedResponse.contentAsString().trim();
+ boolean isBinary =
+ contentType.toLowerCase(Locale.ENGLISH).startsWith(MediaType.OCTET_STREAM.toString());
+ String content = isBinary ? "" : encodedResponse.contentAsString().trim();
+
if (HTTP_BAD_METHOD == encodedResponse.getStatus()) {
response.setState("unknown command");
response.setStatus(ErrorCodes.UNKNOWN_COMMAND);
response.setValue(content);
} else if (HTTP_GATEWAY_TIMEOUT == encodedResponse.getStatus()
|| HTTP_BAD_GATEWAY == encodedResponse.getStatus()) {
response.setState("unknown error");
response.setStatus(ErrorCodes.UNHANDLED_ERROR);
response.setValue(content);
} else {
+ if (isBinary || content.isEmpty()) {
+ response.setState("unknown error");
+ response.setStatus(ErrorCodes.UNHANDLED_ERROR);
+ response.setValue(String.format("Non-textual error response (status %s, Content-Type: %s)", encodedResponse.getStatus(), contentType));
+ return response;
+ }
Map<String, Object> org = json.toType(content, MAP_TYPE);
-...
+ populateResponse(encodedResponse, response, org);
+ }
+ return response;
+ }
+
response.setState("success");
response.setStatus(ErrorCodes.SUCCESS);
- if (contentType.startsWith(MediaType.OCTET_STREAM.toString())) {
+ if (contentType.toLowerCase(Locale.ENGLISH).startsWith(MediaType.OCTET_STREAM.toString())) {
+ // Keep as stream supplier so clients can stream large files
response.setValue(encodedResponse.getContent());
return response;
}
String content = encodedResponse.contentAsString().trim();
if (!content.isEmpty()) {
- if (contentType.startsWith("application/json")) {
+ if (contentType.toLowerCase(Locale.ENGLISH).startsWith("application/json")) {
Map<String, Object> parsed = json.toType(content, MAP_TYPE);
if (parsed.containsKey("value")) {
+ populateResponse(encodedResponse, response, parsed);
+ } else {
+ response.setValue(parsed);
+ }
+ } else {
+ response.setValue(content);
+ }
+ }
+ return response;
+}
+private void populateResponse(HttpResponse encodedResponse, Response response, Map<String, Object> map) {
+ if (map.containsKey("sessionId")) {
+ Object sid = map.get("sessionId");
+ if (sid != null) {
+ response.setSessionId(String.valueOf(sid));
+ }
+ }
+ response.setValue(map.get("value"));
+}
+Suggestion importance[1-10]: 8
__
Why: The suggestion correctly identifies that setting a Contents.Supplier as a Response value is incorrect for JSON-based clients and that error handling for binary content is fragile. The proposed changes improve the robustness of response decoding, especially for binary streams and error conditions.
| PR 16624 (2025-11-21) |
[possible issue] Prevent unintended workflow cancellations
✅ Prevent unintended workflow cancellations
To prevent unintended cancellations between scheduled and push-triggered workflows, update the concurrency group to include the event type by adding ${{ github.event_name }}.
.github/workflows/ci.yml [13-15]
concurrency:
- group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }}
+ group: ${{ github.workflow }}-${{ github.event_name }}-${{ github.event.pull_request.number || github.ref }}
cancel-in-progress: trueSuggestion importance[1-10]: 8
__
Why: The suggestion correctly identifies a subtle but important issue where scheduled and push-triggered workflows would cancel each other, and the proposed fix using ${{ github.event_name }} is the correct solution.
| PR 16623 (2025-11-21) |
[general] Align type hints with behavior
✅ Align type hints with behavior
Update the type hint for the device_pixel_ratio parameter in set_viewport to float | None | UNDEFINED to match the implementation that accepts None for resetting the value.
py/selenium/webdriver/common/bidi/browsing_context.py [984-990]
def set_viewport(
self,
context: str | None = None,
viewport: dict | None | UNDEFINED = UNDEFINED,
- device_pixel_ratio: float | UNDEFINED = UNDEFINED,
+ device_pixel_ratio: float | None | UNDEFINED = UNDEFINED,
user_contexts: list[str] | None = None,
) -> None:Suggestion importance[1-10]: 5
__
Why: The suggestion correctly identifies that the type hint for device_pixel_ratio is inconsistent with the implementation, which explicitly handles None as a valid input to reset the value.
[possible issue] Fix incorrect variable assignment bug
✅ Fix incorrect variable assignment bug
In set_viewport, fix the incorrect assignment to params["devicePixelRatio"] by using the device_pixel_ratio variable instead of viewport.
py/selenium/webdriver/common/bidi/browsing_context.py [1011-1016]
if device_pixel_ratio is UNDEFINED:
pass
elif device_pixel_ratio is None:
params["devicePixelRatio"] = None
else:
- params["devicePixelRatio"] = viewport
+ params["devicePixelRatio"] = device_pixel_ratioSuggestion importance[1-10]: 10
__
Why: The suggestion correctly identifies a critical copy-paste bug that assigns the wrong variable, breaking the set_viewport functionality for the device_pixel_ratio parameter.
[possible issue] Correct a test assertion typo
✅ Correct a test assertion typo
In test_set_viewport_back_to_default, correct the assertion for viewport height to compare against the default height (default_viewport_size[1]) instead of the default width.
py/test/selenium/webdriver/common/bidi_browsing_context_tests.py [402-404]
assert viewport_size[0] == default_viewport_size[0]
-assert viewport_size[1] == default_viewport_size[0]
+assert viewport_size[1] == default_viewport_size[1]
assert device_pixel_ratio == default_device_pixel_ratioSuggestion importance[1-10]: 9
__
Why: The suggestion correctly identifies a copy-paste error in a test assertion, which would cause the test to validate the wrong behavior and potentially pass incorrectly.
| PR 16620 (2025-11-20) |
[general] Improve filtering logic for performance
✅ Improve filtering logic for performance
Refactor the label filtering logic to use a Set for grouping keys instead of a List and multiple if conditions, improving lookup performance and code clarity.
java/src/org/openqa/selenium/grid/node/docker/DockerOptions.java [269-289]
// Get custom grouping labels from configuration
List<String> customLabelKeys =
config.getAll(DOCKER_SECTION, "grouping-labels").orElseGet(Collections::emptyList);
+
+Set<String> groupingKeys = new HashSet<>(customLabelKeys);
+groupingKeys.add("com.docker.compose.project");
+groupingKeys.add("io.podman.compose.project");
Map<String, String> allLabels = info.get().getLabels();
// Filter for project/grouping labels that work across orchestration systems
// Keep only project identifiers, exclude service-specific labels to prevent
// exit monitoring in Docker Compose, Podman Compose, etc.
return allLabels.entrySet().stream()
- .filter(
- entry -> {
- String key = entry.getKey();
- // Docker Compose project label
- if (key.equals("com.docker.compose.project")) return true;
- // Podman Compose project label
- if (key.equals("io.podman.compose.project")) return true;
- // Custom user-defined grouping labels
- if (customLabelKeys.contains(key)) return true;
- return false;
- })
+ .filter(entry -> groupingKeys.contains(entry.getKey()))
.collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue));Suggestion importance[1-10]: 6
__
Why: The suggestion correctly proposes using a Set for more efficient lookups, which improves performance and code readability, aligning with best practices.
| PR 16616 (2025-11-20) |
[general] Avoid code duplication by linking files
✅ Avoid code duplication by linking files
Remove the duplicated StringSyntaxAttribute.cs file and instead link to the single source file from the project file to avoid code duplication.
dotnet/src/support/Internal/StringSyntaxAttribute.cs [1-88]
-// <copyright file="StringSyntaxAttribute.cs" company="Selenium Committers">
-// Licensed to the Software Freedom Conservancy (SFC) under one
-// or more contributor license agreements. See the NOTICE file
-// distributed with this work for additional information
-// regarding copyright ownership. The SFC licenses this file
-// to you under the Apache License, Version 2.0 (the
-// "License"); you may not use this file except in compliance
-// with the License. You may obtain a copy of the License at
-//
-// http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing,
-// software distributed under the License is distributed on an
-// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
-// KIND, either express or implied. See the License for the
-// specific language governing permissions and limitations
-// under the License.
-// </copyright>
+// This file should be deleted.
-#if !NET8_0_OR_GREATER
-
-namespace System.Diagnostics.CodeAnalysis;
-
-/// <summary>Specifies the syntax used in a string.</summary>
-[AttributeUsage(AttributeTargets.Parameter | AttributeTargets.Field | AttributeTargets.Property, AllowMultiple = false, Inherited = false)]
-internal sealed class StringSyntaxAttribute : Attribute
-{
- /// <summary>Initializes the <see cref="StringSyntaxAttribute"/> with the identifier of the syntax used.</summary>
- /// <param name="syntax">The syntax identifier.</param>
- public StringSyntaxAttribute(string syntax)
- {
- Syntax = syntax;
- Arguments = [];
- }
-
- /// <summary>Initializes the <see cref="StringSyntaxAttribute"/> with the identifier of the syntax used.</summary>
- /// <param name="syntax">The syntax identifier.</param>
- /// <param name="arguments">Optional arguments associated with the specific syntax employed.</param>
- public StringSyntaxAttribute(string syntax, params object?[] arguments)
- {
- Syntax = syntax;
- Arguments = arguments;
- }
-
- /// <summary>Gets the identifier of the syntax used.</summary>
- public string Syntax { get; }
-
- /// <summary>Optional arguments associated with the specific syntax employed.</summary>
- public object?[] Arguments { get; }
-
- /// <summary>The syntax identifier for strings containing composite formats for string formatting.</summary>
- public const string CompositeFormat = nameof(CompositeFormat);
-
- /// <summary>The syntax identifier for strings containing date format specifiers.</summary>
- public const string DateOnlyFormat = nameof(DateOnlyFormat);
-
- /// <summary>The syntax identifier for strings containing date and time format specifiers.</summary>
- public const string DateTimeFormat = nameof(DateTimeFormat);
-
- /// <summary>The syntax identifier for strings containing <see cref="Enum"/> format specifiers.</summary>
- public const string EnumFormat = nameof(EnumFormat);
-
- /// <summary>The syntax identifier for strings containing <see cref="Guid"/> format specifiers.</summary>
- public const string GuidFormat = nameof(GuidFormat);
-
- /// <summary>The syntax identifier for strings containing JavaScript Object Notation (JSON).</summary>
- public const string Json = nameof(Json);
-
- /// <summary>The syntax identifier for strings containing numeric format specifiers.</summary>
- public const string NumericFormat = nameof(NumericFormat);
-
- /// <summary>The syntax identifier for strings containing regular expressions.</summary>
- public const string Regex = nameof(Regex);
-
- /// <summary>The syntax identifier for strings containing time format specifiers.</summary>
- public const string TimeOnlyFormat = nameof(TimeOnlyFormat);
-
- /// <summary>The syntax identifier for strings containing <see cref="TimeSpan"/> format specifiers.</summary>
- public const string TimeSpanFormat = nameof(TimeSpanFormat);
-
- /// <summary>The syntax identifier for strings containing URIs.</summary>
- public const string Uri = nameof(Uri);
-
- /// <summary>The syntax identifier for strings containing XML.</summary>
- public const string Xml = nameof(Xml);
-}
-
-#endif
-Suggestion importance[1-10]: 6
__
Why: The suggestion correctly identifies that the PR introduces a duplicated file and proposes a standard .NET practice to resolve it, which improves code maintainability.
| PR 16613 (2025-11-19) |
[possible issue] Avoid mutating a constructor parameter
✅ Avoid mutating a constructor parameter
To avoid mutating a constructor parameter and prevent potential runtime exceptions, create a new mutable copy of the composeLabels map before adding the new label.
java/src/org/openqa/selenium/grid/node/docker/DockerSessionFactory.java [144-146]
-this.composeLabels = Require.nonNull("Docker Compose labels", composeLabels);
// Merge compose labels with oneoff=False to prevent triggering --exit-code-from dynamic grid
-this.composeLabels.put("com.docker.compose.oneoff", "False");
+Map<String, String> allLabels = new HashMap<>(Require.nonNull("Docker Compose labels", composeLabels));
+allLabels.put("com.docker.compose.oneoff", "False");
+this.composeLabels = Collections.unmodifiableMap(allLabels);Suggestion importance[1-10]: 8
__
Why: The suggestion correctly identifies a potential UnsupportedOperationException if an immutable map is passed to the constructor and avoids a side effect by not mutating the input parameter, which significantly improves the code's robustness.
| PR 16610 (2025-11-18) |
[possible issue] Revert breaking lockfile version change
✅ Revert breaking lockfile version change
Revert the lockfileVersion update from '6.0' to '9.0'. This change requires a pnpm major version upgrade (to v9+) across all environments, which could break dependency installation if not coordinated.
-lockfileVersion: '9.0'
+lockfileVersion: '6.0'Suggestion importance[1-10]: 10
__
Why: This suggestion correctly points out a critical, project-wide breaking change in the pnpm lockfileVersion, which could halt all development and CI/CD pipelines if not managed correctly.
| PR 16607 (2025-11-17) |
[general] Ensure consistent docstring quote style
✅ Ensure consistent docstring quote style
Update the docstring function to generate docstrings with double quotes (""") instead of single quotes (''') for consistency.
def docstring(description):
""" Generate a docstring from a description. """
if not description:
return ''
description = escape_backticks(description)
- return dedent("'''\n{}\n'''").format(description)
+ return dedent('"""\n{}\n"""').format(description)Suggestion importance[1-10]: 7
__
Why: The suggestion correctly points out that while the PR changes existing docstrings to use double quotes, the docstring generator function itself still produces single-quoted docstrings, which is inconsistent with the PR's intent.
| PR 16603 (2025-11-16) |
[general] Remove redundant client-side context filtering
✅ Remove redundant client-side context filtering
Remove the redundant client-side context check in OnEntryAddedAsync methods. The event subscription is already filtered by context at the protocol level, making the if statement unnecessary.
dotnet/src/webdriver/BiDi/BrowsingContext/BrowsingContextLogModule.cs [28-48]
public Task<Subscription> OnEntryAddedAsync(Func<Log.LogEntry, Task> handler, ContextSubscriptionOptions? options = null)
{
- return logModule.OnEntryAddedAsync(async args =>
- {
- if (args.Source.Context?.Equals(context) is true)
- {
- await handler(args).ConfigureAwait(false);
- }
- }, options.WithContext(context));
+ return logModule.OnEntryAddedAsync(handler, options.WithContext(context));
}
public Task<Subscription> OnEntryAddedAsync(Action<Log.LogEntry> handler, ContextSubscriptionOptions? options = null)
{
- return logModule.OnEntryAddedAsync(args =>
- {
- if (args.Source.Context?.Equals(context) is true)
- {
- handler(args);
- }
- }, options.WithContext(context));
+ return logModule.OnEntryAddedAsync(handler, options.WithContext(context));
}Suggestion importance[1-10]: 6
__
Why: The suggestion correctly identifies redundant client-side filtering logic that is now handled by the protocol-level subscription, simplifying the code and improving maintainability.
| PR 16601 (2025-11-15) |
[possible issue] Correct JSON serialization for optional parameters
✅ Correct JSON serialization for optional parameters
To align with the WebDriver BiDi specification, correct the JSON serialization by removing the JsonIgnore attribute from the Viewport property and adding it to the DevicePixelRatio property.
dotnet/src/webdriver/BiDi/BrowsingContext/SetViewportCommand.cs [28]
-internal sealed record SetViewportParameters(BrowsingContext Context, [property: JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)] Viewport? Viewport, double? DevicePixelRatio) : Parameters;
+internal sealed record SetViewportParameters(BrowsingContext Context, Viewport? Viewport, [property: JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)] double? DevicePixelRatio) : Parameters;Suggestion importance[1-10]: 5
__
Why: The suggestion correctly identifies that the PR's change to Viewport is incorrect, as it prevents resetting the viewport by sending a null value. However, the proposed fix of moving the attribute to DevicePixelRatio is also incorrect, as the BiDi spec requires sending null to reset that value as well.
| PR 16599 (2025-11-15) |
[possible issue] Sanitize browser name for container
✅ Sanitize browser name for container
Sanitize the browser name by replacing characters invalid for Docker container names to prevent potential creation failures.
java/src/org/openqa/selenium/grid/node/docker/DockerSessionFactory.java [314-316]
-// Generate container name: browser-<browserName>-<timestamp>-<uuid>
-String browserName = stereotype.getBrowserName().toLowerCase();
+// Generate container name: browser-<browserName>-<sessionIdentifier>
+String browserName = stereotype.getBrowserName().toLowerCase().replaceAll("[^a-z0-9_.-]", "-");
String containerName = String.format("browser-%s-%s", browserName, sessionIdentifier);Suggestion importance[1-10]: 8
__
Why: The suggestion correctly identifies that the browser name is not sanitized before being used in a Docker container name, which could lead to container creation failures if the name contains invalid characters.
[learned best practice] Validate and encode URL parameter
✅ Validate and encode URL parameter
Validate and URL-encode the optional container name to avoid invalid requests and injection issues. Reject empty/whitespace-only names and percent-encode the value.
java/src/org/openqa/selenium/docker/client/CreateContainer.java [66-69]
String url = String.format("/v%s/containers/create", apiVersion);
-if (info.getName() != null && !info.getName().isEmpty()) {
- url += "?name=" + info.getName();
+String name = info.getName();
+if (name != null) {
+ name = name.trim();
+ if (!name.isEmpty()) {
+ String encoded = java.net.URLEncoder.encode(name, java.nio.charset.StandardCharsets.UTF_8);
+ url += "?name=" + encoded;
+ }
}Suggestion importance[1-10]: 6
__
Why: Relevant best practice - Guard external API and I/O operations with targeted validation and proper encoding when building request URLs.
| PR 16596 (2025-11-14) |
[possible issue] Avoid modifying nested map directly
✅ Avoid modifying nested map directly
To avoid side effects, create a defensive copy of the networkSettings map in adaptContainerInspectResponse before modifying it, ensuring the original response data structure remains unchanged.
java/src/org/openqa/selenium/docker/client/V148Adapter.java [165-205]
@SuppressWarnings("unchecked")
-Map<String, Object> networkSettings = (Map<String, Object>) adapted.get("NetworkSettings");
+Map<String, Object> originalNetworkSettings =
+ (Map<String, Object>) adapted.get("NetworkSettings");
-if (networkSettings != null) {
+if (originalNetworkSettings != null) {
+ // Create a mutable copy to avoid modifying the original map
+ Map<String, Object> networkSettings = new HashMap<>(originalNetworkSettings);
+
@SuppressWarnings("unchecked")
Map<String, Object> networks = (Map<String, Object>) networkSettings.get("Networks");
if (networks != null) {
for (Map.Entry<String, Object> entry : networks.entrySet()) {
if (entry.getValue() instanceof Map) {
@SuppressWarnings("unchecked")
Map<String, Object> network = (Map<String, Object>) entry.getValue();
if (network.containsKey("GwPriority")) {
LOG.fine(
"Network '" + entry.getKey() + "' includes GwPriority (API v" + apiVersion + ")");
}
}
}
}
// Remove deprecated fields (should not be present in v1.48+)
String[] deprecatedFields = {
"HairpinMode",
"LinkLocalIPv6Address",
"LinkLocalIPv6PrefixLen",
"SecondaryIPAddresses",
"SecondaryIPv6Addresses",
"Bridge" // Deprecated in v1.51, removed in v1.52
};
for (String field : deprecatedFields) {
if (networkSettings.containsKey(field)) {
LOG.fine(
"Removing deprecated field '"
+ field
+ "' from NetworkSettings (deprecated in earlier API versions)");
networkSettings.remove(field);
}
}
+ adapted.put("NetworkSettings", networkSettings);
}Suggestion importance[1-10]: 6
__
Why: The suggestion correctly identifies that a nested map is being modified, which could lead to side effects. Creating a defensive copy is good practice for immutability and prevents unintended modification of the original data structure.
| PR 16594 (2025-11-14) |
[learned best practice] Close context in finally
✅ Close context in finally
Wrap context creation/usage in try-finally and close the BrowsingContext in finally to avoid leaks if assertions throw.
java/test/org/openqa/selenium/bidi/emulation/SetGeolocationOverrideTest.java [201-235]
BrowsingContext context = new BrowsingContext(driver, driver.getWindowHandle());
-String contextId = context.getId();
-...
-// Error because there's no real geolocation available
-assertThat(r2.containsKey("error")).isTrue();
+try {
+ String contextId = context.getId();
+ String url = appServer.whereIsSecure("blank.html");
+ context.navigate(url, ReadinessState.COMPLETE);
+ driver.switchTo().window(context.getId());
+ String origin =
+ (String) ((JavascriptExecutor) driver).executeScript("return window.location.origin;");
+ Emulation emul = new Emulation(driver);
+ GeolocationCoordinates coords = new GeolocationCoordinates(37.7749, -122.4194);
+ emul.setGeolocationOverride(
+ new SetGeolocationOverrideParameters(coords).contexts(List.of(contextId)));
+ Object firstResult = getBrowserGeolocation(driver, null, origin);
+ Map<String, Object> r1 = ((Map<String, Object>) firstResult);
+ assertThat(r1.containsKey("error")).isFalse();
+ double latitude1 = ((Number) r1.get("latitude")).doubleValue();
+ double longitude1 = ((Number) r1.get("longitude")).doubleValue();
+ assertThat(abs(latitude1 - coords.getLatitude())).isLessThan(0.0001);
+ assertThat(abs(longitude1 - coords.getLongitude())).isLessThan(0.0001);
+ emul.setGeolocationOverride(
+ new SetGeolocationOverrideParameters((GeolocationCoordinates) null)
+ .contexts(List.of(contextId)));
+ Object secondResult = getBrowserGeolocation(driver, null, origin);
+ Map<String, Object> r2 = ((Map<String, Object>) secondResult);
+ assertThat(r2.containsKey("error")).isTrue();
+} finally {
+ context.close();
+}Suggestion importance[1-10]: 6
__
Why: Relevant best practice - Ensure resources are cleaned up using try-finally or equivalent to close/tear down contexts and user contexts, even on exceptions.
| PR 16564 (2025-11-08) |
[possible issue] Fix incorrect target file paths
✅ Fix incorrect target file paths
Fix typos in the target attribute of file elements for the net462 framework in Selenium.WebDriver.StrongNamed.nuspec by removing the extra dot in the filenames.
dotnet/src/webdriver/Selenium.WebDriver.StrongNamed.nuspec [43-45]
-<file src="lib/net462/WebDriver.StrongNamed.dll" target="lib/net462/WebDriverStrongNamed..dll" />
-<file src="lib/net462/WebDriver.StrongNamed.pdb" target="lib/net462/WebDriverStrongNamed..pdb" />
-<file src="lib/net462/WebDriver.StrongNamed.xml" target="lib/net462/WebDriverStrongNamed..xml" />
+<file src="lib/net462/WebDriver.StrongNamed.dll" target="lib/net462/WebDriver.StrongNamed.dll" />
+<file src="lib/net462/WebDriver.StrongNamed.pdb" target="lib/net462/WebDriver.StrongNamed.pdb" />
+<file src="lib/net462/WebDriver.StrongNamed.xml" target="lib/net462/WebDriver.StrongNamed.xml" />Suggestion importance[1-10]: 9
__
Why: The suggestion correctly identifies a typo in the NuGet package specification that would lead to incorrectly named files, breaking the package for the net462 target.
| PR 16556 (2025-11-06) |
[general] Replace fixed sleep with explicit wait
✅ Replace fixed sleep with explicit wait
Replace time.sleep(1) with an explicit WebDriverWait to create a more reliable test for verifying that a file is not downloaded.
py/test/selenium/webdriver/common/bidi_browser_tests.py [300-305]
driver.find_element(By.ID, "file-1").click()
-time.sleep(1)
+try:
+ WebDriverWait(driver, 2, poll_frequency=0.2).until(lambda _: len(os.listdir(tmp_dir)) > 0)
+ # If we get here, a file was created, which is a failure
+ files = os.listdir(tmp_dir)
+ pytest.fail(f"A file was downloaded unexpectedly: {files}")
+except TimeoutException:
+ # This is the expected outcome, no file was downloaded within the timeout.
+ pass
-files = os.listdir(tmp_dir)
-assert len(files) == 0, f"No files should be downloaded when denied, but found: {files}"
-Suggestion importance[1-10]: 6
__
Why: The suggestion correctly identifies that using time.sleep() can lead to flaky tests and proposes a more robust explicit wait, which improves test reliability.
| PR 16551 (2025-11-04) |
[general] Make required arguments explicit in signature
✅ Make required arguments explicit in signature
Update the ChromiumDriver.init method signature to make browser_name and vendor_prefix required arguments, removing the Optional type hint and default None value, and then remove the now-redundant runtime checks for None.
py/selenium/webdriver/chromium/webdriver.py [31-67]
def __init__(
self,
- browser_name: Optional[str] = None,
- vendor_prefix: Optional[str] = None,
+ browser_name: str,
+ vendor_prefix: str,
options: Optional[ChromiumOptions] = None,
service: Optional[ChromiumService] = None,
keep_alive: bool = True,
) -> None:
"""Create a new WebDriver instance, start the service, and create new ChromiumDriver instance.
Args:
browser_name: Browser name used when matching capabilities.
vendor_prefix: Company prefix to apply to vendor-specific WebDriver extension commands.
options: This takes an instance of ChromiumOptions.
service: Service object for handling the browser driver if you need to pass extra details.
keep_alive: Whether to configure ChromiumRemoteConnection to use HTTP keep-alive.
"""
self.service = service if service else ChromiumService()
options = options if options else ChromiumOptions()
finder = DriverFinder(self.service, options)
if finder.get_browser_path():
options.binary_location = finder.get_browser_path()
options.browser_version = None
self.service.path = self.service.env_path() or finder.get_driver_path()
self.service.start()
- if browser_name is None:
- raise ValueError("browser_name must be specified")
- if vendor_prefix is None:
- raise ValueError("vendor_prefix must be specified")
-
executor = ChromiumRemoteConnection(
remote_server_addr=self.service.service_url,
browser_name=browser_name,
vendor_prefix=vendor_prefix,Suggestion importance[1-10]: 7
__
Why: The suggestion correctly identifies a contradiction between the method signature, which marks arguments as optional, and the implementation, which requires them. Aligning the signature makes the API contract clearer and enables static analysis tools to catch errors.
| PR 16530 (2025-10-29) |
[general] Return a defensive copy of map
✅ Return a defensive copy of map
Return a defensive copy of the internal map in the toMap() method to prevent external modification and maintain encapsulation.
java/src/org/openqa/selenium/bidi/emulation/AbstractOverrideParameters.java [57]
-return map;
+return new HashMap<>(map);Suggestion importance[1-10]: 8
__
Why: The suggestion correctly identifies a significant encapsulation issue in the new AbstractOverrideParameters class where returning a direct reference to the internal map allows bypassing validation logic. Providing a defensive copy is crucial for maintaining the object's integrity.
[possible issue] Create a defensive copy of parameters
✅ Create a defensive copy of parameters
To ensure the Command object is immutable, create a defensive, unmodifiable copy of the params map that allows null values.
java/src/org/openqa/selenium/bidi/Command.java [51]
-this.params = Require.nonNull("Command parameters", params);
+this.params = java.util.Collections.unmodifiableMap(new java.util.HashMap<>(Require.nonNull("Command parameters", params)));Suggestion importance[1-10]: 7
__
Why: The PR removed Map.copyOf() which made the params map immutable but disallowed nulls. This suggestion correctly identifies that the new implementation is mutable from the outside and proposes a solution that restores immutability while still allowing null values, improving the robustness of the Command object.
| PR 16511 (2025-10-25) |
[general] Correct docstring return type
✅ Correct docstring return type
Correct the return type in the web_socket_url docstring from bool to str to accurately reflect that it returns a URL.
py/selenium/webdriver/common/options.py [316-328]
web_socket_url = _BaseOptionsDescriptor("webSocketUrl")
"""Gets and Sets WebSocket URL.
Usage:
- Get: `self.web_socket_url`
- Set: `self.web_socket_url = value`
Args:
value: str
Returns:
- bool when getting, None when setting.
+ str when getting, None when setting.
"""Suggestion importance[1-10]: 3
__
Why: The suggestion correctly identifies and fixes a documentation error where the return type for web_socket_url is incorrectly stated as bool instead of str, improving the accuracy of the docstring.
| PR 16504 (2025-10-24) |
[learned best practice] Add robust test cleanup
✅ Add robust test cleanup
Ensure the tab and user context are cleaned up even if assertions fail by wrapping them in try/finally blocks.
py/test/selenium/webdriver/common/bidi_emulation_tests.py [347-363]
def test_set_locale_override_with_user_contexts(driver, pages, value):
"""Test setting locale override with user contexts."""
user_context = driver.browser.create_user_context()
+ try:
+ context_id = driver.browsing_context.create(type=WindowTypes.TAB, user_context=user_context)
+ try:
+ driver.switch_to.window(context_id)
+ driver.emulation.set_locale_override(locale=value, user_contexts=[user_context])
+ driver.browsing_context.navigate(context_id, pages.url("formPage.html"), wait="complete")
+ current_locale = get_browser_locale(driver)
+ assert current_locale == value, f"Expected locale {value}, got {current_locale}"
+ finally:
+ driver.browsing_context.close(context_id)
+ finally:
+ driver.browser.remove_user_context(user_context)
- context_id = driver.browsing_context.create(type=WindowTypes.TAB, user_context=user_context)
-
- driver.switch_to.window(context_id)
-
- driver.emulation.set_locale_override(locale=value, user_contexts=[user_context])
-
- driver.browsing_context.navigate(context_id, pages.url("formPage.html"), wait="complete")
-
- current_locale = get_browser_locale(driver)
- assert current_locale == value, f"Expected locale {value}, got {current_locale}"
-
- driver.browsing_context.close(context_id)
- driver.browser.remove_user_context(user_context)
-Suggestion importance[1-10]: 6
__
Why: Relevant best practice - Enforce deterministic cleanup of created resources in tests to avoid leaks and cross-test interference.
| PR 16487 (2025-10-22) |
[learned best practice] Guard message fields before use
✅ Guard message fields before use
Add defensive checks to ensure message['params'] is a Hash before use and safely duplicate callbacks to prevent nil errors and races when handlers are removed concurrently.
rb/lib/selenium/webdriver/common/websocket_connection.rb [137-147]
while (frame = incoming_frame.next)
break if @closing
message = process_frame(frame)
- next unless message['method']
+ method = message['method']
+ next unless method.is_a?(String)
params = message['params']
- @messages_mtx.synchronize { callbacks[message['method']].dup }.each do |callback|
+ next unless params.is_a?(Hash)
+
+ handlers = @callbacks_mtx.synchronize { callbacks[method].dup }
+ handlers.each do |callback|
@callback_threads.add(callback_thread(params, &callback))
end
endSuggestion importance[1-10]: 6
__
Why: Relevant best practice - Validate inputs and states early to avoid nil errors; guard message handling and callback spawning against missing fields.
[high-level] Refactor to use multiple specialized mutexes
✅ Refactor to use multiple specialized mutexes
**payload) @@ -98,12 +106,10 @@ begin socket.write(out_frame.to_s) rescue *CONNECTION_ERRORS => e
-
raise Error::WebDriverError, "WebSocket is closed (#{e.class}: #{e.message})" -
end -
wait.until do -
@mtx.synchronize { messages.delete(id) } -
end
-
raise e, "WebSocket is closed (#{e.class}: #{e.message})" -
end -
wait.until { @messages_mtx.synchronize { messages.delete(id) } } end private
@@ -132,9 +138,8 @@ message = process_frame(frame) next unless message['method']
-
params = message['params'] -
@mtx.synchronize { callbacks[message['method']].dup }.each do |callback| -
@callback_threads.add(callback_thread(params, &callback))
-
@messages_mtx.synchronize { callbacks[message['method']].dup }.each do |callback| -
@callback_threads.add(callback_thread(message['params'], &callback)) end end end
@@ -154,7 +159,7 @@ return {} if message.empty?
msg = JSON.parse(message)
-
@mtx.synchronize { messages[msg['id']] = msg if msg.key?('id') }
-
@messages_mtx.synchronize { messages[msg['id']] = msg if msg.key?('id') } WebDriver.logger.debug "WebSocket <- #{msg}"[...MAX_LOG_MESSAGE_SIZE], id: :ws msg
@@ -170,7 +175,8 @@ rescue Error::WebDriverError, *CONNECTION_ERRORS => e WebDriver.logger.debug "Callback aborted: #{e.class}: #{e.message}", id: :ws rescue StandardError => e
-
# Unexpected handler failure; log with a short backtrace.
-
return if @closing -
bt = Array(e.backtrace).first(5).join("\n") WebDriver.logger.error "Callback error: #{e.class}: #{e.message}\n#{bt}", id: :ws end
**Replace the single, coarse-grained mutex with multiple, more granular mutexes, one for each shared resource (@callbacks, @messages, @closing). This will reduce lock contention and improve concurrency.**
### Examples:
rb/lib/selenium/webdriver/common/websocket_connection.rb [40]
```ruby
@mtx = Mutex.new
rb/lib/selenium/webdriver/common/websocket_connection.rb [77-80]
@mtx.synchronize do
callbacks[event] << block
block.object_id
endclass WebSocketConnection
def initialize(url:)
@mtx = Mutex.new
@closing = false
@callbacks = {}
@messages = {}
# ...
end
def add_callback(event, █)
@mtx.synchronize do
# ... access @callbacks
end
end
def send_cmd(**payload)
# ...
wait.until do
@mtx.synchronize { messages.delete(id) }
end
end
def process_frame(frame)
# ...
@mtx.synchronize { messages[msg['id']] = msg }
# ...
end
endclass WebSocketConnection
def initialize(url:)
@callbacks_mtx = Mutex.new
@messages_mtx = Mutex.new
@closing_mtx = Mutex.new
@closing = false
@callbacks = {}
@messages = {}
# ...
end
def add_callback(event, █)
@callbacks_mtx.synchronize do
# ... access @callbacks
end
end
def send_cmd(**payload)
# ...
wait.until do
@messages_mtx.synchronize { messages.delete(id) }
end
end
def process_frame(frame)
# ...
@messages_mtx.synchronize { messages[msg['id']] = msg }
# ...
end
endSuggestion importance[1-10]: 8
__
Why: The suggestion correctly identifies that a single coarse-grained mutex is used to protect multiple independent resources, which can cause unnecessary lock contention and performance issues. Adopting a more granular locking strategy is a significant architectural improvement for the concurrency model introduced in this PR.
[possible issue] Ensure session is always deleted
✅ Ensure session is always deleted
Modify the quit method to ensure super is always called, even if bidi.close fails. Wrap only the bidi.close call in a begin/rescue block to prevent leaving orphaned browser sessions.
rb/lib/selenium/webdriver/remote/bidi_bridge.rb [48-53]
def quit
- bidi.close
+ begin
+ bidi.close
+ rescue *QUIT_ERRORS
+ nil
+ end
super
-rescue *QUIT_ERRORS
- nil
endSuggestion importance[1-10]: 8
__
Why: The suggestion correctly identifies a critical flaw where a failure in bidi.close would prevent the super call from executing, potentially leaving browser sessions running. The proposed fix ensures the session is always cleaned up.
[possible issue] Fix race condition in callback removal
✅ Fix race condition in callback removal
Refactor the remove_callback method to use a single synchronize block. This will prevent a race condition by ensuring the callback removal and error message data collection are performed atomically.
rb/lib/selenium/webdriver/common/websocket_connection.rb [84-88]
-removed = @mtx.synchronize { callbacks[event].reject! { |cb| cb.object_id == id } }
-return if removed || @closing
+@mtx.synchronize do
+ return if @closing
-ids = @mtx.synchronize { callbacks[event]&.map(&:object_id) }
-raise Error::WebDriverError, "Callback with ID #{id} does not exist for event #{event}: #{ids}"
+ callbacks_for_event = callbacks[event]
+ if callbacks_for_event.reject! { |cb| cb.object_id == id }
+ return
+ end
+ ids = callbacks_for_event.map(&:object_id)
+ raise Error::WebDriverError, "Callback with ID #{id} does not exist for event #{event}: #{ids}"
+end
+Suggestion importance[1-10]: 7
__
Why: The suggestion correctly identifies a race condition in the remove_callback method where two separate synchronize blocks could lead to inconsistent state, and proposes a valid fix by using a single atomic block.
| PR 16477 (2025-10-21) |
[possible issue] Fix off-by-one error in slicing
✅ Fix off-by-one error in slicing
Correct the string slicing from f[:-7] to f[:-8] to properly remove the _spec.rb suffix from test file names.
rb/spec/integration/selenium/webdriver/BUILD.bazel [56-63]
[
rb_integration_test(
- name = f[:-7],
+ name = f[:-8],
srcs = [f],
tags = ["bidi"],
)
for f in _BIDI_FILES
]Suggestion importance[1-10]: 9
__
Why: The suggestion correctly identifies a functional bug in the build script. The incorrect string slicing f[:-7] would generate malformed test target names, likely causing build failures or tests not being correctly identified.
[learned best practice] Remove early macro return
✅ Remove early macro return
Avoid returning early from the macro; guard only the BiDi target creation with a conditional so other targets remain unaffected and macro flow stays predictable.
-if "bidi" not in tags:
- return # don't create -bidi targets for non-BiDi tests
+if "bidi" in tags:
+ rb_test(
+ name = "{}-{}-bidi".format(name, browser),
+ size = "large",
+ srcs = srcs,
+ # ...
+ )Suggestion importance[1-10]: 6
__
Why: Relevant best practice - Keep build macros pure and predictable by avoiding early returns that skip subsequent target generation unexpectedly; prefer structured conditionals so all intended targets are considered.
| PR 16473 (2025-10-20) |
[possible issue] Fix conditional logic for GitHub Actions
✅ Fix conditional logic for GitHub Actions
Fix the conditional logic for ignoring tests on GitHub Actions. The current implementation incorrectly ignores tests when the gitHubActions attribute is not specified.
java/test/org/openqa/selenium/testing/IgnoreComparator.java [52-56]
return ignoreList.anyMatch(
driver ->
(ignored.contains(driver.value()) || driver.value() == Browser.ALL)
- && ((!driver.gitHubActions() || TestUtilities.isOnGitHubActions()))
+ && (!driver.gitHubActions() || TestUtilities.isOnGitHubActions())
&& isOpen(driver.issue()));Suggestion importance[1-10]: 10
__
Why: The suggestion correctly identifies a critical logic bug in the shouldIgnore method. The current logic causes tests to be ignored incorrectly when @Ignore is used without gitHubActions=true, which is a significant flaw introduced by the PR.
| PR 16449 (2025-10-16) |
[possible issue] Simplify file serving by reading all files in binary mode
✅ Simplify file serving by reading all files in binary mode
Simplify the _serve_file method by opening all files in binary mode ("rb") to avoid unnecessary and potentially corrupting text re-encoding.
py/test/selenium/webdriver/common/webserver.py [72-87]
def _serve_file(self, file_path):
"""Serve a file from the HTML root directory."""
- content_type, _ = mimetypes.guess_type(file_path)
+ with open(file_path, "rb") as f:
+ return f.read()
- if content_type and (
- content_type.startswith("image/")
- or content_type.startswith("application/")
- or content_type.startswith("video/")
- or content_type.startswith("audio/")
- ):
- with open(file_path, "rb") as f:
- return f.read()
- else:
- # text files
- with open(file_path, encoding="latin-1") as f:
- return f.read().encode("utf-8")
-Suggestion importance[1-10]: 7
__
Why: The suggestion correctly identifies that re-encoding text files from latin-1 to utf-8 is problematic and can cause corruption. The proposed change to read all files in binary mode is simpler, more robust, and the correct approach for a web server.
[learned best practice] Default Content-Type when unknown
✅ Default Content-Type when unknown
Ensure a valid Content-Type is always sent by defaulting to a sensible MIME type when guessing fails. This avoids sending a None header value.
py/test/selenium/webdriver/common/webserver.py [104-108]
elif os.path.isfile(file_path):
content_type, _ = mimetypes.guess_type(file_path)
+ if not content_type:
+ content_type = "application/octet-stream"
content = self._serve_file(file_path)
self._send_response(content_type)
self.wfile.write(content)Suggestion importance[1-10]: 6
__
Why: Relevant best practice - Validate inputs and states early, providing safe defaults to prevent logic errors and None-related issues.
| PR 16444 (2025-10-15) |
[incremental [*]] Prevent heartbeat interval overflow
✅ Prevent heartbeat interval overflow
** * Configures ZeroMQ heartbeat settings on a socket to prevent stale connections.
-
-
-
The heartbeat interval is clamped between 1 second and ~24 days to prevent integer overflow
-
-
- and ensure reasonable values. If the provided duration is outside this range, it will be
-
- adjusted and a warning will be logged.
- @param socket The ZMQ socket to configure
- @param heartbeatPeriod The heartbeat interval duration @@ -39,14 +46,58 @@ */ static void configureHeartbeat(ZMQ.Socket socket, Duration heartbeatPeriod, String socketType) { if (heartbeatPeriod != null && !heartbeatPeriod.isZero() && !heartbeatPeriod.isNegative()) {
-
int heartbeatIvl = (int) heartbeatPeriod.toMillis();
-
long heartbeatMs = heartbeatPeriod.toMillis(); -
long clampedHeartbeatMs = clampHeartbeatInterval(heartbeatMs, socketType); -
// Safe to cast to int now -
int heartbeatIvl = (int) clampedHeartbeatMs; -
int heartbeatTimeout = heartbeatIvl * 3; -
int heartbeatTtl = heartbeatIvl * 6; -
socket.setHeartbeatIvl(heartbeatIvl);
-
socket.setHeartbeatTimeout(heartbeatIvl * 3); -
socket.setHeartbeatTtl(heartbeatIvl * 6);
-
socket.setHeartbeatTimeout(heartbeatTimeout); -
socket.setHeartbeatTtl(heartbeatTtl); -
LOG.info( String.format(
-
"ZMQ %s socket heartbeat configured: interval=%dms, timeout=%dms, ttl=%dms", -
socketType, heartbeatIvl, heartbeatIvl * 3, heartbeatIvl * 6));
-
"ZMQ %s socket heartbeat configured: interval=%ds, timeout=%ds, ttl=%ds", -
} }
socketType, heartbeatIvl / 1000, heartbeatTimeout / 1000, heartbeatTtl / 1000)); - /**
-
- Clamps the heartbeat interval to safe bounds and logs warnings if adjustments are made.
-
-
- @param heartbeatMs The heartbeat interval in milliseconds
-
- @param socketType The socket type for logging
-
- @return The clamped heartbeat interval
- */
- private static long clampHeartbeatInterval(long heartbeatMs, String socketType) {
- if (heartbeatMs < MIN_HEARTBEAT_MS) {
-
logHeartbeatClampWarning(socketType, heartbeatMs, MIN_HEARTBEAT_MS, "below minimum"); -
return MIN_HEARTBEAT_MS; - }
- if (heartbeatMs > MAX_HEARTBEAT_MS) {
-
logHeartbeatClampWarning(socketType, heartbeatMs, MAX_HEARTBEAT_MS, "exceeds maximum"); -
return MAX_HEARTBEAT_MS; - }
- return heartbeatMs;
- }
- /**
-
- Logs a warning when the heartbeat interval is clamped.
-
-
- @param socketType The socket type
-
- @param originalMs The original interval value in milliseconds
-
- @param clampedMs The clamped interval value in milliseconds
-
- @param reason The reason for clamping
- */
- private static void logHeartbeatClampWarning(
-
String socketType, long originalMs, long clampedMs, String reason) { - LOG.log(
-
Level.WARNING, -
String.format( -
"ZMQ %s socket heartbeat interval %ds %s %ds, clamping to %ds", -
socketType, originalMs / 1000, reason, clampedMs / 1000, clampedMs / 1000)); - }
**In configureHeartbeat, guard against integer overflow by clamping the heartbeat interval in milliseconds to a safe maximum before casting to int and setting socket options. Also, enforce a sensible minimum value.**
[java/src/org/openqa/selenium/events/zeromq/ZmqUtils.java [40-52]](https://github.com/SeleniumHQ/selenium/pull/16444/files#diff-f9f1bd9c11ff8d0995d73085dc4b3e906a8aa15c1a5ba90fc6b9059a3a8080a1R40-R52)
```diff
static void configureHeartbeat(ZMQ.Socket socket, Duration heartbeatPeriod, String socketType) {
- if (heartbeatPeriod != null && !heartbeatPeriod.isZero() && !heartbeatPeriod.isNegative()) {
- int heartbeatIvl = (int) heartbeatPeriod.toMillis();
- socket.setHeartbeatIvl(heartbeatIvl);
- socket.setHeartbeatTimeout(heartbeatIvl * 3);
- socket.setHeartbeatTtl(heartbeatIvl * 6);
- LOG.info(
- String.format(
- "ZMQ %s socket heartbeat configured: interval=%dms, timeout=%dms, ttl=%dms",
- socketType, heartbeatIvl, heartbeatIvl * 3, heartbeatIvl * 6));
+ if (heartbeatPeriod == null || heartbeatPeriod.isZero() || heartbeatPeriod.isNegative()) {
+ return;
}
+ long ivlMsLong = heartbeatPeriod.toMillis();
+ // Enforce sane bounds: at least 100ms and at most Integer.MAX_VALUE / 6 to avoid overflow below
+ long min = 100L;
+ long max = Integer.MAX_VALUE / 6L;
+ if (ivlMsLong < min) {
+ ivlMsLong = min;
+ } else if (ivlMsLong > max) {
+ ivlMsLong = max;
+ }
+ int heartbeatIvl = (int) ivlMsLong;
+ int heartbeatTimeout = heartbeatIvl * 3;
+ int heartbeatTtl = heartbeatIvl * 6;
+ socket.setHeartbeatIvl(heartbeatIvl);
+ socket.setHeartbeatTimeout(heartbeatTimeout);
+ socket.setHeartbeatTtl(heartbeatTtl);
+ LOG.info(
+ String.format(
+ "ZMQ %s socket heartbeat configured: interval=%dms, timeout=%dms, ttl=%dms",
+ socketType, heartbeatIvl, heartbeatTimeout, heartbeatTtl));
}
Suggestion importance[1-10]: 8
__
Why: The suggestion correctly identifies a potential integer overflow when calculating heartbeat intervals from a large Duration, which could lead to invalid socket configuration and runtime errors.
[general] Refactor duplicated code into a utility method
✅ Refactor duplicated code into a utility method
Extract the duplicated configureHeartbeat method from BoundZmqEventBus and UnboundZmqEventBus into a static helper method in a new utility class to avoid code repetition and improve maintainability.
java/src/org/openqa/selenium/events/zeromq/BoundZmqEventBus.java [84-97]
-private void configureHeartbeat(ZMQ.Socket socket, Duration heartbeatPeriod, String socketType) {
- if (heartbeatPeriod != null && !heartbeatPeriod.isZero() && !heartbeatPeriod.isNegative()) {
- int heartbeatIvl = (int) heartbeatPeriod.toMillis();
- socket.setHeartbeatIvl(heartbeatIvl);
- // Set heartbeat timeout to 3x the interval
- socket.setHeartbeatTimeout(heartbeatIvl * 3);
- // Set heartbeat TTL to 6x the interval
- socket.setHeartbeatTtl(heartbeatIvl * 6);
- LOG.info(
- String.format(
- "Event bus %s socket heartbeat configured: interval=%dms, timeout=%dms, ttl=%dms",
- socketType, heartbeatIvl, heartbeatIvl * 3, heartbeatIvl * 6));
+// In a new file: java/src/org/openqa/selenium/events/zeromq/ZmqUtils.java
+package org.openqa.selenium.events.zeromq;
+
+import java.time.Duration;
+import java.util.logging.Logger;
+import org.zeromq.ZMQ;
+
+class ZmqUtils {
+ private static final Logger LOG = Logger.getLogger(ZmqUtils.class.getName());
+
+ private ZmqUtils() {
+ // Utility class
+ }
+
+ static void configureHeartbeat(ZMQ.Socket socket, Duration heartbeatPeriod, String socketType) {
+ if (heartbeatPeriod != null && !heartbeatPeriod.isZero() && !heartbeatPeriod.isNegative()) {
+ long periodMillis = heartbeatPeriod.toMillis();
+ if (periodMillis > Integer.MAX_VALUE) {
+ LOG.warning(
+ String.format(
+ "Heartbeat period %dms is too large. Capping at %dms.",
+ periodMillis, Integer.MAX_VALUE));
+ periodMillis = Integer.MAX_VALUE;
+ }
+ int heartbeatIvl = (int) periodMillis;
+ socket.setHeartbeatIvl(heartbeatIvl);
+ // Set heartbeat timeout to 3x the interval
+ socket.setHeartbeatTimeout(heartbeatIvl * 3);
+ // Set heartbeat TTL to 6x the interval
+ socket.setHeartbeatTtl(heartbeatIvl * 6);
+ LOG.info(
+ String.format(
+ "Event bus %s socket heartbeat configured: interval=%dms, timeout=%dms, ttl=%dms",
+ socketType, heartbeatIvl, heartbeatIvl * 3, heartbeatIvl * 6));
+ }
}
}
+// In BoundZmqEventBus.java, replace the configureHeartbeat method with:
+private void configureHeartbeat(ZMQ.Socket socket, Duration heartbeatPeriod, String socketType) {
+ ZmqUtils.configureHeartbeat(socket, heartbeatPeriod, socketType);
+}
+Suggestion importance[1-10]: 6
__
Why: The suggestion correctly identifies that the configureHeartbeat method is duplicated in BoundZmqEventBus and UnboundZmqEventBus, and proposes a valid refactoring to a shared utility method, which improves code maintainability by adhering to the DRY principle.
| PR 16442 (2025-10-15) |
[possible issue] Fix incorrect test assertion order
✅ Fix incorrect test assertion order
In the RSpec tests, move the action (e.g., network.continue_request) before the expect(...).to have_received(...) assertion to correctly verify the behavior.
rb/spec/unit/selenium/webdriver/bidi/network_spec.rb [35-41]
it 'sends only the mandatory request ID when all optional args are nil' do
expected_payload = {request: request_id}
+ network.continue_request(id: request_id)
+
expect(mock_bidi).to have_received(:send_cmd).with('network.continueRequest', expected_payload)
-
- network.continue_request(id: request_id)
endSuggestion importance[1-10]: 9
__
Why: The suggestion correctly identifies a critical flaw in the test implementation where the assertion precedes the action, rendering all new tests ineffective at verifying the intended behavior.
| PR 16427 (2025-10-14) |
[possible issue] Remove global docstring check ignore
✅ Remove global docstring check ignore
Remove the global ignore rule for docstring (D) checks from pyproject.toml to ensure the linter can report docstring issues as intended.
[tool.ruff.lint.per-file-ignores]
-"*.py" = ["D"]Suggestion importance[1-10]: 8
__
Why: The suggestion correctly identifies that the per-file-ignores configuration completely negates the enabling of docstring checks, making the primary change of this PR ineffective.
| PR 16421 (2025-10-13) |
[general] Consolidate browser skip logic
✅ Consolidate browser skip logic
Consolidate the separate if conditions for skipping the test on Chrome and Edge into a single condition using in for better readability and maintainability.
py/test/selenium/webdriver/common/bidi_network_tests.py [104-107]
-if driver.caps["browserName"] == "chrome":
- pytest.skip(reason="Request handlers don't yet work in Chrome when using classic navigation")
-if driver.caps["browserName"] == "edge":
- pytest.skip(reason="Request handlers don't yet work in Edge when using classic navigation")
+if driver.caps["browserName"] in ("chrome", "edge"):
+ pytest.skip(reason="Request handlers don't yet work in Chrome/Edge when using classic navigation")Suggestion importance[1-10]: 4
__
Why: The suggestion improves code conciseness and maintainability by consolidating two if statements into a single check, which is a good practice but has a low impact on functionality.
| PR 16403 (2025-10-08) |
[possible issue] Use a non-generic base class
✅ Use a non-generic base class
Modify _pendingCommands to use a non-generic base CommandInfo class. This will allow storing different command result types in the dictionary, improving type safety.
dotnet/src/webdriver/BiDi/Communication/Broker.cs [40]
-private readonly ConcurrentDictionary<long, CommandInfo<EmptyResult>> _pendingCommands = new();
+private readonly ConcurrentDictionary<long, CommandInfo> _pendingCommands = new();Suggestion importance[1-10]: 8
__
Why: The suggestion correctly identifies a type safety issue where _pendingCommands is constrained to CommandInfo, which forces an unsafe cast later. Proposing a non-generic base class is a valid and robust design pattern to solve this problem.
| PR 16392 (2025-10-06) |
[possible issue] Remove exception from unimplemented method
Remove exception from unimplemented method
Remove the NotImplementedException from the Initialize method in EmulationModule and other modules. If no initialization is needed, the method body should be empty to prevent runtime crashes.
dotnet/src/webdriver/BiDi/Emulation/EmulationModule.cs [93-96]
protected internal override void Initialize(Broker broker)
{
- throw new NotImplementedException();
+ // No-op for this module.
}Suggestion importance[1-10]: 9
__
Why: This suggestion correctly identifies a critical bug introduced by the PR. The Initialize method will be called upon module creation, and throwing a NotImplementedException will cause a runtime crash, making the modules unusable.
[general] Register module-specific JSON serialization context
Register module-specific JSON serialization context
Implement the Initialize method in BrowsingContextModule to register a module-specific JsonSerializerContext. This aligns with the pattern in BrowserModule and is necessary for proper JSON serialization.
dotnet/src/webdriver/BiDi/BrowsingContext/BrowsingContextModule.cs [254-257]
protected internal override void Initialize(Broker broker)
{
-
+ broker.ConfigureJsonContext(opts => opts.TypeInfoResolverChain.Add(BrowsingContextModuleJsonSerializerContext.Default));
}Suggestion importance[1-10]: 8
__
Why: The suggestion correctly points out that the PR's refactoring goal is to register module-specific JSON contexts, which is missing for BrowsingContextModule. Implementing this is crucial for completing the refactoring and gaining the performance/AOT benefits of source-generated serialization.
| PR 16386 (2025-10-05) |
[possible issue] Fix incorrect enum serialization casing
✅ Fix incorrect enum serialization casing
The UseStringEnumConverter option serializes enums to PascalCase, but the BiDi specification requires lowercase. Remove this option and instead implement custom JsonConverters for each enum to ensure correct serialization.
dotnet/src/webdriver/BiDi/Communication/Json/BiDiJsonSerializerContext.cs [189]
-[JsonSourceGenerationOptions(UseStringEnumConverter = true)]
+// This line should be removed.
+// [JsonSourceGenerationOptions(UseStringEnumConverter = true)]Suggestion importance[1-10]: 10
__
Why: The suggestion correctly identifies a critical issue where using UseStringEnumConverter would cause enum serialization to violate the WebDriver BiDi specification (PascalCase vs. lowercase), leading to communication failures.
| PR 16364 (2025-10-01) |
[general] Remove redundant and unused code
✅ Remove redundant and unused code
Remove the redundant file javascript/grid-ui/src/hooks/useTheme.tsx. Its logic is already implemented in javascript/grid-ui/src/contexts/ThemeContext.tsx, and it is not used by the application.
javascript/grid-ui/src/hooks/useTheme.tsx [1-61]
-// Licensed to the Software Freedom Conservancy (SFC) under one
-// or more contributor license agreements. See the NOTICE file
-// distributed with this work for additional information
-// regarding copyright ownership. The SFC licenses this file
-// to you under the Apache License, Version 2.0 (the
-// "License"); you may not use this file except in compliance
-// with the License. You may obtain a copy of the License at
-//
-// http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing,
-// software distributed under the License is distributed on an
-// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
-// KIND, either express or implied. See the License for the
-// specific language governing permissions and limitations
-// under the License.
+// This file should be deleted.
-import { useState, useEffect } from 'react'
-import { lightTheme, darkTheme } from '../theme/themes'
-
-type ThemeMode = 'light' | 'dark' | 'system'
-
-export const useTheme = () => {
- const [themeMode, setThemeMode] = useState<ThemeMode>('system')
- const [systemPrefersDark, setSystemPrefersDark] = useState(false)
-
- useEffect(() => {
- if (typeof window !== 'undefined') {
- const saved = localStorage.getItem('theme-mode') as ThemeMode
- if (saved) setThemeMode(saved)
- setSystemPrefersDark(window.matchMedia('(prefers-color-scheme: dark)').matches)
- }
- }, [])
-
-
-
- useEffect(() => {
- if (typeof window !== 'undefined') {
- localStorage.setItem('theme-mode', themeMode)
- }
- }, [themeMode])
-
- useEffect(() => {
- if (typeof window !== 'undefined') {
- const mediaQuery = window.matchMedia('(prefers-color-scheme: dark)')
- const handler = (e: MediaQueryListEvent) => setSystemPrefersDark(e.matches)
- mediaQuery.addEventListener('change', handler)
- return () => mediaQuery.removeEventListener('change', handler)
- }
- }, [])
-
- const isDark = themeMode === 'dark' || (themeMode === 'system' && systemPrefersDark)
- const currentTheme = isDark ? darkTheme : lightTheme
-
- return {
- themeMode,
- setThemeMode,
- currentTheme,
- isDark
- }
-}
-Suggestion importance[1-10]: 8
__
Why: The suggestion correctly identifies a redundant file (useTheme.tsx) whose logic is duplicated in ThemeContext.tsx. Removing this unused code improves maintainability and reduces confusion.
[general] Add assertions to validate behavior
✅ Add assertions to validate behavior
Add assertions to the 'cycles through theme modes on click' test in ThemeToggle.test.tsx. The test currently performs clicks without verifying the component's state changes.
javascript/grid-ui/src/tests/components/ThemeToggle.test.tsx [36-56]
it('cycles through theme modes on click', () => {
render(
<CustomThemeProvider>
<ThemeToggle />
</CustomThemeProvider>
)
-
+
const button = screen.getByRole('button')
-
- // Should start with system mode
- expect(button).toHaveAttribute('aria-label', 'Toggle theme')
-
- // Click to light mode
+
+ // Starts in system mode, next is light
+ expect(screen.getByTestId('AutoModeIcon')).toBeInTheDocument()
fireEvent.click(button)
-
- // Click to dark mode
+
+ // Now in light mode, next is dark
+ expect(screen.getByTestId('LightModeIcon')).toBeInTheDocument()
fireEvent.click(button)
-
- // Click back to system mode
+
+ // Now in dark mode, next is system
+ expect(screen.getByTestId('DarkModeIcon')).toBeInTheDocument()
fireEvent.click(button)
+
+ // Back to system mode
+ expect(screen.getByTestId('AutoModeIcon')).toBeInTheDocument()
})Suggestion importance[1-10]: 7
__
Why: The suggestion correctly identifies that the test case is missing assertions for intermediate states, which makes it ineffective. Improving the test coverage is a valuable enhancement.
[possible issue] Fix incorrect test mock setup
✅ Fix incorrect test mock setup
In TopBar.test.tsx, remove the incorrect mock for ../../hooks/useTheme. Instead, wrap the TopBar component with CustomThemeProvider to align the test environment with its actual usage.
javascript/grid-ui/src/tests/components/TopBar.test.tsx [23-43]
-// Mock the useTheme hook
-jest.mock('../../hooks/useTheme', () => ({
- useTheme: () => ({
- themeMode: 'light',
- setThemeMode: jest.fn(),
- currentTheme: {},
- isDark: false
- })
-}))
+import { CustomThemeProvider } from '../../contexts/ThemeContext'
const user = userEvent.setup()
+
+const renderWithTheme = (ui: React.ReactElement) => {
+ return render(<CustomThemeProvider>{ui}</CustomThemeProvider>)
+}
it('renders basic information', () => {
const subheaderText = 'Hello, world!'
const handleClick = jest.fn()
- render(<TopBar subheader={subheaderText} drawerOpen
+ renderWithTheme(<TopBar subheader={subheaderText} drawerOpen
toggleDrawer={handleClick}/>)
expect(screen.getByText('Selenium Grid')).toBeInTheDocument()
expect(screen.getByRole('img')).toHaveAttribute('alt', 'Selenium Grid Logo')
expect(screen.getByText(subheaderText)).toBeInTheDocument()
})Suggestion importance[1-10]: 8
__
Why: The suggestion correctly points out that the test is mocking a redundant hook (useTheme) instead of using the CustomThemeProvider as the application does, leading to an incorrect test setup.
[learned best practice] Guard browser API accesses
✅ Guard browser API accesses
Add environment checks before using window, localStorage, and matchMedia to avoid runtime errors in SSR/tests and ensure safe access. Fallback gracefully when unavailable.
javascript/grid-ui/src/contexts/ThemeContext.tsx [40-55]
useEffect(() => {
- const saved = localStorage.getItem('theme-mode') as ThemeMode
- if (saved) setThemeMode(saved)
- setSystemPrefersDark(window.matchMedia('(prefers-color-scheme: dark)').matches)
+ if (typeof window !== 'undefined') {
+ try {
+ const saved = window.localStorage.getItem('theme-mode') as ThemeMode
+ if (saved) setThemeMode(saved)
+ const mq = window.matchMedia?.('(prefers-color-scheme: dark)')
+ setSystemPrefersDark(!!mq && mq.matches)
+ } catch {
+ // ignore storage access errors
+ }
+ }
}, [])
useEffect(() => {
- localStorage.setItem('theme-mode', themeMode)
+ if (typeof window !== 'undefined') {
+ try {
+ window.localStorage.setItem('theme-mode', themeMode)
+ } catch {
+ // ignore storage access errors
+ }
+ }
}, [themeMode])
useEffect(() => {
- const mediaQuery = window.matchMedia('(prefers-color-scheme: dark)')
- const handler = (e: MediaQueryListEvent) => setSystemPrefersDark(e.matches)
- mediaQuery.addEventListener('change', handler)
- return () => mediaQuery.removeEventListener('change', handler)
+ if (typeof window !== 'undefined' && typeof window.matchMedia === 'function') {
+ const mediaQuery = window.matchMedia('(prefers-color-scheme: dark)')
+ const handler = (e: MediaQueryListEvent) => setSystemPrefersDark(e.matches)
+ mediaQuery.addEventListener?.('change', handler)
+ return () => mediaQuery.removeEventListener?.('change', handler)
+ }
+ return
}, [])Suggestion importance[1-10]: 6
__
Why:
Relevant best practice - Validate inputs and states early to prevent logic errors; guard access to browser-only APIs (window, localStorage, matchMedia) for non-browser/test environments.
| PR 16362 (2025-10-01) |
[possible issue] Fix syntax error in expression
✅ Fix syntax error in expression
Remove the extra closing curly brace } from the cache-key value to fix a syntax error in the GitHub Actions expression.
.github/workflows/ci-python.yml [70]
-cache-key: python-unit-test-${{ matrix.os }}}
+cache-key: python-unit-test-${{ matrix.os }}Suggestion importance[1-10]: 9
__
Why: The suggestion correctly identifies a syntax error in the GitHub Actions expression for cache-key which would cause the CI job to fail.
| PR 16347 (2025-09-24) |
[general] Fix inconsistent handler naming
✅ Fix inconsistent handler naming
In BrowsingContextInspector, rename the downloadWillEndMapper and downloadWillEndEvent fields to downloadEndMapper and downloadEndEvent to match the event name browsingContext.downloadEnd, and update their usages accordingly.
java/src/org/openqa/selenium/bidi/module/BrowsingContextInspector.java [68-186]
-private final Function<Map<String, Object>, DownloadEnded> downloadWillEndMapper =
+private final Function<Map<String, Object>, DownloadEnded> downloadEndMapper =
params -> {
try (StringReader reader = new StringReader(JSON.toJson(params));
- JsonInput input = JSON.newInput(reader)) {
+ JsonInput input = JSON.newInput(reader)) {
return input.read(DownloadEnded.class);
}
};
-...
-
-private final Event<DownloadEnded> downloadWillEndEvent =
- new Event<>("browsingContext.downloadEnd", downloadWillEndMapper);
-
-...
+private final Event<DownloadEnded> downloadEndEvent =
+ new Event<>("browsingContext.downloadEnd", downloadEndMapper);
public void onDownloadEnd(Consumer<DownloadEnded> consumer) {
if (browsingContextIds.isEmpty()) {
- this.bidi.addListener(downloadWillEndEvent, consumer);
+ this.bidi.addListener(downloadEndEvent, consumer);
} else {
- this.bidi.addListener(browsingContextIds, downloadWillEndEvent, consumer);
+ this.bidi.addListener(browsingContextIds, downloadEndEvent, consumer);
}
}
+@Override
+public void close() {
+ this.bidi.clearListener(browsingContextCreated);
+ this.bidi.clearListener(browsingContextDestroyed);
+ this.bidi.clearListener(userPromptOpened);
+ this.bidi.clearListener(userPromptClosed);
+ this.bidi.clearListener(historyUpdated);
+ this.bidi.clearListener(downloadWillBeginEvent);
+ this.bidi.clearListener(downloadEndEvent);
+
+ navigationEventSet.forEach(this.bidi::clearListener);
+}
+Suggestion importance[1-10]: 4
__
Why: The suggestion improves code consistency by renaming internal variables like downloadWillEndMapper and downloadWillEndEvent to align with the event name browsingContext.downloadEnd. This enhances readability and maintainability, though it is a minor style improvement.
[general] Refactor to remove duplicated JSON parsing
✅ Refactor to remove duplicated JSON parsing
Refactor the DownloadEnded.fromJson method to eliminate redundant JSON parsing logic by delegating deserialization to the DownloadCanceled.fromJson or DownloadCompleted.fromJson methods based on the status field.
java/src/org/openqa/selenium/bidi/browsingcontext/DownloadEnded.java [30-79]
public static DownloadEnded fromJson(JsonInput input) {
- String browsingContextId = null;
- String navigationId = null;
- long timestamp = 0;
- String url = null;
- String status = null;
- String filepath = null;
+ Map<String, Object> jsonMap = input.read(Map.class);
+ String status = (String) jsonMap.get("status");
- input.beginObject();
- while (input.hasNext()) {
- switch (input.nextName()) {
- case "context":
- browsingContextId = input.read(String.class);
- break;
- case "navigation":
- navigationId = input.read(String.class);
- break;
- case "timestamp":
- timestamp = input.read(Long.class);
- break;
- case "url":
- url = input.read(String.class);
- break;
- case "status":
- status = input.read(String.class);
- break;
- case "filepath":
- filepath = input.read(String.class);
- break;
- default:
- input.skipValue();
- break;
+ try (StringReader reader = new StringReader(new Json().toJson(jsonMap));
+ JsonInput jsonInput = new Json().newInput(reader)) {
+ if ("canceled".equals(status)) {
+ return new DownloadEnded(DownloadCanceled.fromJson(jsonInput));
+ } else if ("complete".equals(status)) {
+ return new DownloadEnded(DownloadCompleted.fromJson(jsonInput));
+ } else {
+ throw new IllegalArgumentException(
+ "status must be either 'canceled' or 'complete', but got: " + status);
}
- }
- input.endObject();
-
- // Create the appropriate object based on status
- if ("canceled".equals(status)) {
- DownloadCanceled canceled =
- new DownloadCanceled(browsingContextId, navigationId, timestamp, url, status);
- return new DownloadEnded(canceled);
- } else if ("complete".equals(status)) {
- DownloadCompleted completed =
- new DownloadCompleted(browsingContextId, navigationId, timestamp, url, status, filepath);
- return new DownloadEnded(completed);
- } else {
- throw new IllegalArgumentException(
- "status must be either 'canceled' or 'complete', but got: " + status);
}
}Suggestion importance[1-10]: 6
__
Why: The suggestion correctly identifies duplicated JSON parsing logic in DownloadEnded.fromJson and proposes a valid refactoring to delegate deserialization, which improves code structure and maintainability.
[learned best practice] Use accurate event naming
✅ Use accurate event naming
Rename downloadWillEndEvent to reflect the actual event name (downloadEnd) for clarity and consistency with other events. This avoids confusion and aligns with event semantics.
java/src/org/openqa/selenium/bidi/module/BrowsingContextInspector.java [97-247]
-private final Event<DownloadEnded> downloadWillEndEvent =
+private final Event<DownloadEnded> downloadEndEvent =
new Event<>("browsingContext.downloadEnd", downloadWillEndMapper);
...
public void onDownloadEnd(Consumer<DownloadEnded> consumer) {
if (browsingContextIds.isEmpty()) {
- this.bidi.addListener(downloadWillEndEvent, consumer);
+ this.bidi.addListener(downloadEndEvent, consumer);
} else {
- this.bidi.addListener(browsingContextIds, downloadWillEndEvent, consumer);
+ this.bidi.addListener(browsingContextIds, downloadEndEvent, consumer);
}
}
...
public void close() {
this.bidi.clearListener(browsingContextCreated);
this.bidi.clearListener(browsingContextDestroyed);
this.bidi.clearListener(userPromptOpened);
this.bidi.clearListener(userPromptClosed);
this.bidi.clearListener(historyUpdated);
this.bidi.clearListener(downloadWillBeginEvent);
- this.bidi.clearListener(downloadWillEndEvent);
+ this.bidi.clearListener(downloadEndEvent);
navigationEventSet.forEach(this.bidi::clearListener);
}Suggestion importance[1-10]: 6
__
Why: Relevant best practice - Always close or dispose resources and clear listeners to prevent leaks; ensure new listeners are cleared on shutdown using consistent fields.
| PR 16338 (2025-09-20) |
[possible issue] Prevent resource leaks with caching
✅ Prevent resource leaks with caching
Cache the BiDi instance to prevent creating a new HTTP client and connection on every invocation. This avoids potential resource leaks and performance issues.
java/src/org/openqa/selenium/bidi/BiDiProvider.java [47-56]
-return () -> {
- URI wsUri = getBiDiUrl(caps).orElseThrow(() -> new BiDiException("BiDi not supported"));
-
- HttpClient.Factory clientFactory = HttpClient.Factory.createDefault();
- ClientConfig wsConfig = ClientConfig.defaultConfig().baseUri(wsUri);
- HttpClient wsClient = clientFactory.createClient(wsConfig);
- Connection connection = new Connection(wsClient, wsUri.toString());
-
- return Optional.of(new BiDi(connection));
+return new HasBiDi() {
+ private volatile Optional<BiDi> cachedBiDi;
+
+ @Override
+ public Optional<BiDi> getBiDi() {
+ if (cachedBiDi == null) {
+ synchronized (this) {
+ if (cachedBiDi == null) {
+ URI wsUri = getBiDiUrl(caps).orElseThrow(() -> new BiDiException("BiDi not supported"));
+ HttpClient.Factory clientFactory = HttpClient.Factory.createDefault();
+ ClientConfig wsConfig = ClientConfig.defaultConfig().baseUri(wsUri);
+ HttpClient wsClient = clientFactory.createClient(wsConfig);
+ Connection connection = new Connection(wsClient, wsUri.toString());
+ cachedBiDi = Optional.of(new BiDi(connection));
+ }
+ }
+ }
+ return cachedBiDi;
+ }
};Suggestion importance[1-10]: 8
__
Why: The suggestion correctly identifies a significant issue where new HTTP clients and connections are created on each call, which can lead to resource exhaustion and performance degradation. The proposed caching solution is a robust fix for this problem.
[possible issue] Prevent connection recreation with caching
✅ Prevent connection recreation with caching
Cache the DevTools instance to avoid recreating connections and performing version matching on every call. This prevents potential resource leaks and unnecessary overhead.
java/src/org/openqa/selenium/devtools/DevToolsProvider.java [45-53]
-return () -> {
- Object cdpVersion = caps.getCapability("se:cdpVersion");
- String version = cdpVersion instanceof String ? (String) cdpVersion : caps.getBrowserVersion();
-
- CdpInfo info = new CdpVersionFinder().match(version).orElseGet(NoOpCdpInfo::new);
- Optional<DevTools> devTools =
- SeleniumCdpConnection.create(caps).map(conn -> new DevTools(info::getDomains, conn));
- return devTools;
+return new HasDevTools() {
+ private volatile Optional<DevTools> cachedDevTools;
+
+ @Override
+ public Optional<DevTools> getDevTools() {
+ if (cachedDevTools == null) {
+ synchronized (this) {
+ if (cachedDevTools == null) {
+ Object cdpVersion = caps.getCapability("se:cdpVersion");
+ String version = cdpVersion instanceof String ? (String) cdpVersion : caps.getBrowserVersion();
+ CdpInfo info = new CdpVersionFinder().match(version).orElseGet(NoOpCdpInfo::new);
+ cachedDevTools = SeleniumCdpConnection.create(caps).map(conn -> new DevTools(info::getDomains, conn));
+ }
+ }
+ }
+ return cachedDevTools;
+ }
};Suggestion importance[1-10]: 8
__
Why: The suggestion correctly points out that repeatedly creating DevTools connections and performing version matching is inefficient and can lead to resource leaks. The proposed caching mechanism is an effective solution to ensure the connection is created only once.
[possible issue] Prevent multiple connection creation
✅ Prevent multiple connection creation
To prevent resource leaks from multiple connections, cache the BiDi instance so it is created only once. The current implementation creates a new connection every time getBiDi() is called.
java/src/org/openqa/selenium/bidi/BiDiProvider.java [47-56]
-return () -> {
- URI wsUri = getBiDiUrl(caps).orElseThrow(() -> new BiDiException("BiDi not supported"));
-
- HttpClient.Factory clientFactory = HttpClient.Factory.createDefault();
- ClientConfig wsConfig = ClientConfig.defaultConfig().baseUri(wsUri);
- HttpClient wsClient = clientFactory.createClient(wsConfig);
- Connection connection = new Connection(wsClient, wsUri.toString());
-
- return Optional.of(new BiDi(connection));
+return new HasBiDi() {
+ private volatile Optional<BiDi> biDi;
+
+ @Override
+ public Optional<BiDi> getBiDi() {
+ if (biDi == null) {
+ synchronized (this) {
+ if (biDi == null) {
+ URI wsUri = getBiDiUrl(caps).orElseThrow(() -> new BiDiException("BiDi not supported"));
+ HttpClient.Factory clientFactory = HttpClient.Factory.createDefault();
+ ClientConfig wsConfig = ClientConfig.defaultConfig().baseUri(wsUri);
+ HttpClient wsClient = clientFactory.createClient(wsConfig);
+ Connection connection = new Connection(wsClient, wsUri.toString());
+ biDi = Optional.of(new BiDi(connection));
+ }
+ }
+ }
+ return biDi;
+ }
};Suggestion importance[1-10]: 9
__
Why: The suggestion correctly identifies a significant issue where the PR's change to lazy-load the connection would create a new connection on every call, leading to resource leaks. The proposed fix using double-checked locking correctly implements a cached, lazy-initialized connection, which is a critical improvement for correctness and performance.
[general] Simplify lazy-loading with a memoized supplier
✅ Simplify lazy-loading with a memoized supplier
Replace the manual double-checked locking implementation with Guava's Suppliers.memoize to simplify the code and improve robustness for lazy initialization.
java/src/org/openqa/selenium/bidi/BiDiProvider.java [47-68]
return new HasBiDi() {
- private volatile Optional<BiDi> biDi;
+ private final Supplier<Optional<BiDi>> biDiSupplier =
+ Suppliers.memoize(
+ () -> {
+ URI wsUri =
+ getBiDiUrl(caps).orElseThrow(() -> new BiDiException("BiDi not supported"));
+
+ HttpClient.Factory clientFactory = HttpClient.Factory.createDefault();
+ ClientConfig wsConfig = ClientConfig.defaultConfig().baseUri(wsUri);
+ HttpClient wsClient = clientFactory.createClient(wsConfig);
+ Connection connection = new Connection(wsClient, wsUri.toString());
+
+ return Optional.of(new BiDi(connection));
+ });
@Override
public Optional<BiDi> maybeGetBiDi() {
- if (biDi == null) {
- synchronized (this) {
- if (biDi == null) {
- URI wsUri = getBiDiUrl(caps).orElseThrow(() -> new BiDiException("BiDi not supported"));
-
- HttpClient.Factory clientFactory = HttpClient.Factory.createDefault();
- ClientConfig wsConfig = ClientConfig.defaultConfig().baseUri(wsUri);
- HttpClient wsClient = clientFactory.createClient(wsConfig);
- Connection connection = new Connection(wsClient, wsUri.toString());
-
- biDi = Optional.of(new BiDi(connection));
- }
- }
- }
- return biDi;
+ return biDiSupplier.get();
}
};Suggestion importance[1-10]: 7
__
Why: The suggestion correctly identifies that the manual double-checked locking can be replaced by a simpler and more robust library utility (Suppliers.memoize), significantly improving code readability and maintainability.
[learned best practice] Use initialized Optional with DCL
✅ Use initialized Optional with DCL
Initialize devTools to Optional.empty() and use a local variable in the double-checked locking to avoid null checks and publication races.
java/src/org/openqa/selenium/devtools/DevToolsProvider.java [45-64]
return new HasDevTools() {
- private volatile Optional<DevTools> devTools;
+ private volatile Optional<DevTools> devTools = Optional.empty();
@Override
public Optional<DevTools> maybeGetDevTools() {
- if (devTools == null) {
+ Optional<DevTools> local = devTools;
+ if (local.isEmpty()) {
synchronized (this) {
- if (devTools == null) {
+ local = devTools;
+ if (local.isEmpty()) {
Object cdpVersion = caps.getCapability("se:cdpVersion");
String version =
cdpVersion instanceof String ? (String) cdpVersion : caps.getBrowserVersion();
CdpInfo info = new CdpVersionFinder().match(version).orElseGet(NoOpCdpInfo::new);
- this.devTools = SeleniumCdpConnection.create(caps).map(conn -> new DevTools(info::getDomains, conn));
+ local = SeleniumCdpConnection.create(caps).map(conn -> new DevTools(info::getDomains, conn));
+ devTools = local;
}
}
}
- return devTools;
+ return local;
}
};Suggestion importance[1-10]: 5
__
Why: Relevant best practice - Initialize attributes to sane defaults to avoid null/partially-initialized states, especially when using double-checked locking.
| PR 16333 (2025-09-18) |
[general] Restrict constant visibility for better encapsulation
✅ Restrict constant visibility for better encapsulation
Change the ChromiumIgnoreReason constant's visibility from public to private to improve encapsulation, as it is only used within the WebExtensionTest class.
dotnet/test/common/BiDi/WebExtension/WebExtensionTest.cs [30-36]
-public const string ChromiumIgnoreReason = """
+private const string ChromiumIgnoreReason = """
The following test suite wants to set driver arguments via Options, but it breaks CDP/DevTools tests.
The desired arguments (for Chromium only?):
--enable-unsafe-extension-debugging
--remote-debugging-pipe
Ignoring these tests for now. Hopefully https://github.com/SeleniumHQ/selenium/issues/15536 will be resolved soon.
""";Suggestion importance[1-10]: 4
__
Why: The suggestion correctly identifies that the ChromiumIgnoreReason constant can be made private to improve encapsulation, as it is only used within its declaring class.
| PR 16327 (2025-09-16) |
[learned best practice] Add missing @Override annotation
✅ Add missing @Override annotation
Add the @Override annotation to getUpNodes() since it implements the new interface method, enabling compile-time checks.
java/src/org/openqa/selenium/grid/distributor/local/LocalNodeRegistry.java [366-376]
+@Override
public Set<NodeStatus> getUpNodes() {
Lock readLock = this.lock.readLock();
readLock.lock();
try {
return model.getSnapshot().stream()
.filter(node -> UP.equals(node.getAvailability()))
.collect(ImmutableSet.toImmutableSet());
} finally {
readLock.unlock();
}
}Suggestion importance[1-10]: 6
__
Why: Relevant best practice - Explicitly annotate interface implementations with @Override to ensure contract adherence and improve maintainability.
| PR 16325 (2025-09-15) |
[possible issue] Sort events to prevent flaky tests
✅ Sort events to prevent flaky tests
To prevent flaky tests, wait for both download events, sort them by URL to ensure a deterministic order, and then validate the properties of each event.
py/test/selenium/webdriver/common/bidi_browsing_context_tests.py [828-840]
-WebDriverWait(driver, 5).until(lambda d: len(events_received) > 1)
+WebDriverWait(driver, 5).until(lambda d: len(events_received) == 2)
assert len(events_received) == 2
-download_event = events_received[0]
-assert download_event.download_params is not None
-assert download_event.download_params.status == "complete"
-assert download_event.download_params.context == context_id
-assert download_event.download_params.timestamp is not None
-assert "downloads/file_1.txt" in download_event.download_params.url
-# we assert that atleast the str "file_1" is present in the downloaded file since multiple downloads
+# Sort events to have a deterministic order for assertions
+events_received.sort(key=lambda e: e.download_params.url)
+
+download_event_1 = events_received[0]
+assert download_event_1.download_params is not None
+assert download_event_1.download_params.status == "complete"
+assert download_event_1.download_params.context == context_id
+assert download_event_1.download_params.timestamp is not None
+assert "downloads/file_1.txt" in download_event_1.download_params.url
+# we assert that at least the str "file_1" is present in the downloaded file since multiple downloads
# will have numbered suffix like file_1 (1)
-assert "file_1" in download_event.download_params.filepath
+assert "file_1" in download_event_1.download_params.filepath
+download_event_2 = events_received[1]
+assert download_event_2.download_params is not None
+assert download_event_2.download_params.status == "complete"
+assert download_event_2.download_params.context == context_id
+assert download_event_2.download_params.timestamp is not None
+assert "downloads/file_2.txt" in download_event_2.download_params.url
+assert "file_2" in download_event_2.download_params.filepath
+Suggestion importance[1-10]: 9
__
Why: The suggestion correctly identifies a potential for flaky tests due to the non-deterministic order of asynchronous download events and provides a robust solution by sorting the events before assertion, which is a critical improvement for test reliability.
| PR 16309 (2025-09-09) |
[possible issue] Preserve None for optional fields
✅ Preserve None for optional fields
Avoid coercing optional values to strings, since str(None) becomes the literal "None", which will be sent to the remote as an invalid value. Pass these through unchanged so None remains None and only actual strings are forwarded.
py/selenium/webdriver/chromium/webdriver.py [63-69]
executor = ChromiumRemoteConnection(
remote_server_addr=self.service.service_url,
- browser_name=str(browser_name),
- vendor_prefix=str(vendor_prefix),
+ browser_name=browser_name,
+ vendor_prefix=vendor_prefix,
keep_alive=keep_alive,
ignore_proxy=options._ignore_local_proxy,
)Suggestion importance[1-10]: 9
__
Why: The suggestion correctly identifies that casting None to a string results in the literal "None", which is likely an invalid value for browser_name and vendor_prefix, thus fixing a bug introduced in the PR.
| PR 16271 (2025-08-29) |
[general] Properly stop driver before nullifying
✅ Properly stop driver before nullifying
Setting the global variable to None without properly stopping the driver can lead to resource leaks. The driver should be stopped before nullifying the reference to ensure proper cleanup.
if request.node.get_closest_marker("no_driver_after_test"):
+ if selenium_driver is not None:
+ selenium_driver.stop_driver()
selenium_driver = NoneSuggestion importance[1-10]: 7
__
Why: The suggestion correctly identifies that nullifying selenium_driver without stopping it first is fragile and can lead to resource leaks, making the code more robust and consistent with other cleanup logic in the file.
| PR 16262 (2025-08-26) |
[possible issue] Fix broken version specifier
✅ Fix broken version specifier
The version specifier is invalid due to a missing comma between the lower and upper bounds. This will break dependency parsing and resolution. Add a comma to separate the specifiers.
-"websocket-client>=1.8.0<2.0",
+"websocket-client>=1.8.0,<2.0",Suggestion importance[1-10]: 10
__
Why: The suggestion correctly identifies a malformed version specifier for websocket-client that lacks a comma, which would break dependency resolution, making this a critical fix.
[possible issue] Correct malformed dependency range
✅ Correct malformed dependency range
The dependency constraint is malformed because it lacks a comma between specifiers, violating PEP 508. Insert the comma to ensure valid parsing by build tools.
-"websocket-client>=1.8.0<2.0",
+"websocket-client>=1.8.0,<2.0",Suggestion importance[1-10]: 10
__
Why: The suggestion correctly identifies a malformed dependency constraint for websocket-client that violates PEP 508 and would break dependency installation, making this a critical fix.
| PR 16250 (2025-08-24) |
[general] Ensure attribute is always initialized
✅ Ensure attribute is always initialized
Initialize self.log_output with a default to avoid potential mypy "possibly uninitialized" errors when new branches are added. Also fix minor formatting for readability. This keeps the attribute always defined and improves maintainability.
py/selenium/webdriver/common/service.py [60-68]
-self.log_output: Optional[Union[int, IOBase]]
+self.log_output: Optional[Union[int, IOBase]] = None
if isinstance(log_output, str):
self.log_output = cast(IOBase, open(log_output, "a+", encoding="utf-8"))
elif log_output == subprocess.STDOUT:
self.log_output = None
elif log_output is None or log_output == subprocess.DEVNULL:
self.log_output = subprocess.DEVNULL
else:
- self.log_output = cast(Union[int, IOBase],log_output)
+ self.log_output = cast(Union[int, IOBase], log_output)Suggestion importance[1-10]: 3
__
Why: The suggestion to initialize self.log_output at declaration is good practice for robustness and readability, although the existing if/elif/else structure already ensures it is always assigned a value.
| PR 16248 (2025-08-23) |
[learned best practice] Validate WebSocket constructor inputs
✅ Validate WebSocket constructor inputs
Validate the constructor parameters to ensure they are present and of the expected types/ranges. Default or raise clear errors for invalid values to prevent subtle runtime issues.
py/selenium/webdriver/remote/websocket_connection.py [31-39]
class WebSocketConnection:
_max_log_message_size = 9999
def __init__(self, url, timeout, interval):
+ if not url:
+ raise ValueError("WebSocket URL must be provided")
+ if timeout is None or timeout <= 0:
+ raise ValueError("timeout must be a positive number")
+ if interval is None or interval <= 0:
+ raise ValueError("interval must be a positive number")
+
self.callbacks = {}
self.session_id = None
self.url = url
- self.response_wait_timeout = timeout
- self.response_wait_interval = interval
+ self.response_wait_timeout = float(timeout)
+ self.response_wait_interval = float(interval)Suggestion importance[1-10]: 6
__
Why: Relevant best practice - Add null checks and validation for parameters and variables before using them to prevent runtime errors.
| PR 16233 (2025-08-21) |
[learned best practice] Fix incorrect Javadoc reference
✅ Fix incorrect Javadoc reference
The Javadoc text references TargetLocator#alert(), which is unrelated to element screenshots, and misuses "@see" in a @param. Update the description to reflect WebElement.getScreenshotAs and fix parameter docs and punctuation.
java/src/org/openqa/selenium/support/events/WebDriverListener.java [613-622]
/**
- * This action will be performed each time after {@link WebDriver.TargetLocator#alert()} is
- * called.
+ * This method will be called after {@link WebElement#getScreenshotAs(OutputType)} is called.
*
- * @param element - decorated WebElement instance
- * @param target - target type, @see OutputType
- * @param result - object in which is stored information about the screenshot.
- * @param <X> - return type for getScreenshotAs.
+ * @param element the decorated WebElement instance
+ * @param target the target type, {@link OutputType}
+ * @param result the screenshot result
+ * @param <X> the return type for getScreenshotAs
*/
default <X> void afterGetScreenshotAs(WebElement element, OutputType<X> target, X result) {}Suggestion importance[1-10]: 6
__
Why: Relevant best practice - Fix syntax errors, typos, and naming inconsistencies in comments and documentation.
[learned best practice] Correct Javadoc parameter links
✅ Correct Javadoc parameter links
**
-
- This action will be performed each time after {@link WebDriver.TargetLocator#alert()} is
-
- called.
-
- This method will be called after {@link TakesScreenshot#getScreenshotAs(OutputType)} is called.
- @param element - decorated WebElement instance
- @param target - target type, @see OutputType
**In Javadoc, avoid using "@see" within @param descriptions; link the type directly with {@link}. This improves clarity and tooling parsing. Update the parameter docs to use proper links and consistent punctuation.**
[java/src/org/openqa/selenium/support/events/WebDriverListener.java [335-343]](https://github.com/SeleniumHQ/selenium/pull/16233/files#diff-27fd3dec4abffb9b78d39c4f8ca918bcb5cd04926e3d252d2398f561f5ad61ffR335-R343)
```diff
/**
- * This method will be called before {@link TakesScreenshot#getScreenshotAs(OutputType)} is
- * called.
+ * This method will be called before {@link TakesScreenshot#getScreenshotAs(OutputType)} is called.
*
- * @param driver - decorated WebDriver instance
- * @param target - target type, @see OutputType
- * @param <X> - return type for getScreenshotAs.
+ * @param driver the decorated WebDriver instance
+ * @param target the target type, {@link OutputType}
+ * @param <X> the return type for getScreenshotAs
*/
default <X> void beforeGetScreenshotAs(WebDriver driver, OutputType<X> target) {}
Suggestion importance[1-10]: 5
__
Why: Relevant best practice - Fix string formatting and interpolation issues; avoid ambiguous or incorrect Javadoc tags and references.
[learned best practice] Clean up Javadoc references
✅ Clean up Javadoc references
** * This method will be called after {@link TakesScreenshot#getScreenshotAs(OutputType)} is called. *
-
- @param driver - decorated WebDriver instance
-
- @param target - target type, @see OutputType
-
- @param result - object in which is stored information about the screenshot.
-
- @param - return type for getScreenshotAs.
-
- @param driver decorated WebDriver instance
-
- @param target target type, see {@link OutputType}
-
- @param result object that stores the screenshot information
-
- @param return type for getScreenshotAs */ default void afterGetScreenshotAs(WebDriver driver, OutputType target, X result) {}
@@ -607,19 +607,19 @@ * This method will be called before {@link TakesScreenshot#getScreenshotAs(OutputType)} is * called. *
-
- @param element - decorated WebElement instance
-
- @param target - target type, @see OutputType
-
- @param - return type for getScreenshotAs.
-
- @param element decorated WebElement instance
-
- @param target target type, see {@link OutputType}
-
- @param return type for getScreenshotAs */ default void beforeGetScreenshotAs(WebElement element, OutputType target) {}
/** * This method will be called after {@link TakesScreenshot#getScreenshotAs(OutputType)} is called. *
-
- @param element - decorated WebElement instance
-
- @param target - target type, @see OutputType
-
- @param result - object in which is stored information about the screenshot.
-
- @param - return type for getScreenshotAs.
-
- @param element decorated WebElement instance
-
- @param target target type, see {@link OutputType}
-
- @param result result object that stores the screenshot information
-
- @param return type for getScreenshotAs
**Replace "@see" inline in @param descriptions with proper Javadoc links or move them to @see tags. Also clarify generic return type wording and remove trailing periods in parameter descriptions for consistency.**
[java/src/org/openqa/selenium/support/events/WebDriverListener.java [335-624]](https://github.com/SeleniumHQ/selenium/pull/16233/files#diff-27fd3dec4abffb9b78d39c4f8ca918bcb5cd04926e3d252d2398f561f5ad61ffR335-R624)
```diff
/**
- * This method will be called before {@link TakesScreenshot#getScreenshotAs(OutputType)} is
- * called.
+ * This method will be called before {@link TakesScreenshot#getScreenshotAs(OutputType)} is called.
*
- * @param driver - decorated WebDriver instance
- * @param target - target type, @see OutputType
- * @param <X> - return type for getScreenshotAs.
+ * @param driver decorated WebDriver instance
+ * @param target target type, see {@link OutputType}
+ * @param <X> return type for getScreenshotAs
*/
default <X> void beforeGetScreenshotAs(WebDriver driver, OutputType<X> target) {}
/**
* This method will be called after {@link TakesScreenshot#getScreenshotAs(OutputType)} is called.
*
- * @param driver - decorated WebDriver instance
- * @param target - target type, @see OutputType
- * @param result - object in which is stored information about the screenshot.
- * @param <X> - return type for getScreenshotAs.
+ * @param driver decorated WebDriver instance
+ * @param target target type, see {@link OutputType}
+ * @param result object that stores the screenshot information
+ * @param <X> return type for getScreenshotAs
*/
default <X> void afterGetScreenshotAs(WebDriver driver, OutputType<X> target, X result) {}
/**
- * This method will be called before {@link TakesScreenshot#getScreenshotAs(OutputType)} is
- * called.
+ * This method will be called before {@link TakesScreenshot#getScreenshotAs(OutputType)} is called.
*
- * @param element - decorated WebElement instance
- * @param target - target type, @see OutputType
- * @param <X> - return type for getScreenshotAs.
+ * @param element decorated WebElement instance
+ * @param target target type, see {@link OutputType}
+ * @param <X> return type for getScreenshotAs
*/
default <X> void beforeGetScreenshotAs(WebElement element, OutputType<X> target) {}
/**
* This method will be called after {@link TakesScreenshot#getScreenshotAs(OutputType)} is called.
*
- * @param element - decorated WebElement instance
- * @param target - target type, @see OutputType
- * @param result - object in which is stored information about the screenshot.
- * @param <X> - return type for getScreenshotAs.
+ * @param element decorated WebElement instance
+ * @param target target type, see {@link OutputType}
+ * @param result object that stores the screenshot information
+ * @param <X> return type for getScreenshotAs
*/
default <X> void afterGetScreenshotAs(WebElement element, OutputType<X> target, X result) {}
Suggestion importance[1-10]: 6
__
Why: Relevant best practice - Fix Javadoc formatting and reference style to avoid ambiguity and maintain consistent documentation quality.
[general] Fix misleading parameter name
✅ Fix misleading parameter name
The parameter name driver for WebElement callbacks is misleading and contradicts the interface, risking confusion and incorrect usage. Rename it to element to align with WebDriverListener method signatures and improve test clarity.
java/test/org/openqa/selenium/support/events/EventFiringDecoratorTest.java [476-483]
-public <X> void beforeGetScreenshotAs(WebElement driver, OutputType<X> target) {
+public <X> void beforeGetScreenshotAs(WebElement element, OutputType<X> target) {
acc.append("beforeGetScreenshotAs ").append(target).append("\n");
}
@Override
-public <X> void afterGetScreenshotAs(WebElement driver, OutputType<X> target, X result) {
+public <X> void afterGetScreenshotAs(WebElement element, OutputType<X> target, X result) {
acc.append("afterGetScreenshotAs ").append(target).append(" ").append(result).append("\n");
}Suggestion importance[1-10]: 4
__
Why: The suggestion correctly identifies a misleading parameter name driver for a WebElement and suggests renaming it to element for consistency with the interface, improving code clarity.
| PR 16228 (2025-08-21) |
[possible issue] Avoid unintended copies with multi-RID
✅ Avoid unintended copies with multi-RID
Guard against custom or cross-targeting builds where RID is set via TargetFramework or RuntimeIdentifiers by also checking that '$(RuntimeIdentifiers)' is empty. This prevents duplicate or incorrect asset copying when NuGet already selects runtime-specific assets.
dotnet/src/webdriver/assets/nuget/build/netstandard2.0/Selenium.WebDriver.targets [4-9]
<!-- Only run if the consumer did NOT set a RID (so NuGet won't select runtimes assets),
and only for .NET Framework projects where this problem is common. -->
-<ItemGroup Condition="'$(RuntimeIdentifier)' == '' and '$(TargetFrameworkIdentifier)' == '.NETFramework'">
+<ItemGroup Condition="'$(RuntimeIdentifier)' == '' and '$(RuntimeIdentifiers)' == '' and '$(TargetFrameworkIdentifier)' == '.NETFramework'">
<Content Include="$(MSBuildThisFileDirectory)..\..\runtimes\win\native\selenium-manager.exe">
<Link>runtimes\win\native\%(Filename)%(Extension)</Link>
<CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>Suggestion importance[1-10]: 7
__
Why: The suggestion correctly identifies that checking only $(RuntimeIdentifier) is insufficient, as $(RuntimeIdentifiers) can be used for multi-targeted builds, making the proposed condition more robust and aligned with the PR's intent.
| PR 16212 (2025-08-19) |
[learned best practice] Close HTTP response via context manager
✅ Close HTTP response via context manager
Ensure the HTTP response object is properly closed by using a context manager. This prevents potential resource leaks under heavy usage or exceptions.
py/selenium/webdriver/common/utils.py [150-151]
-res = urllib.request.urlopen(f"{scheme}://{host}:{port}/status")
-return res.getcode() == 200
+with urllib.request.urlopen(f"{scheme}://{host}:{port}/status") as res:
+ return res.getcode() == 200Suggestion importance[1-10]: 6
__
Why: Relevant best practice - Use proper resource disposal patterns with context managers to prevent leaks when working with network/IO resources.
| PR 16205 (2025-08-18) |
[general] Mark removed API as obsolete error
✅ Mark removed API as obsolete error
Remove UnsubscribeByAttributesOptions since attribute-based unsubscribe is no longer supported, preventing accidental usage and API drift. If kept for binary compatibility, mark it [Obsolete(true)] to produce compile-time errors.
dotnet/src/webdriver/BiDi/Session/UnsubscribeCommand.cs [32-35]
+[Obsolete("Unsubscribe by attributes is no longer supported. Use unsubscribe by subscription id instead.", true)]
public sealed class UnsubscribeByAttributesOptions : CommandOptions
{
public IEnumerable<BrowsingContext.BrowsingContext>? Contexts { get; set; }
}Suggestion importance[1-10]: 7
__
Why: The PR removes the functionality associated with UnsubscribeByAttributesOptions but leaves the class in the public API, so marking it as obsolete improves API consistency and prevents misuse.
| PR 16191 (2025-08-16) |
[possible issue] Fix malformed bug report link
✅ Fix malformed bug report link
Fix the malformed sentence and extra parenthesis to avoid confusing readers and broken rendering. Combine the text and link properly, using a single closing parenthesis and clear wording.
-Issues shouldn't be used for support. To raise a bug, please go here-
-(https://github.com/SeleniumHQ/selenium/issues)).
+Issues shouldn't be used for support. To report a bug, please open an issue at: https://github.com/SeleniumHQ/selenium/issuesSuggestion importance[1-10]: 7
__
Why: The suggestion correctly identifies and fixes a malformed sentence and link, which has an extra parenthesis and unusual punctuation, improving the clarity and correctness of the documentation.
[general] Clarify Slack channel reference
✅ Clarify Slack channel reference
Clarify the Slack reference by using a direct, descriptive link label and avoid bare URLs in parentheses for better readability and accessibility. This reduces ambiguity and improves formatting consistency.
-Discussion of high level project ideas or non-technical topics should
-move to the Selenium Slack channel (https://inviter.co/seleniumhq).
+Discussion of high level project ideas or non-technical topics should move to the Selenium Slack channel: https://inviter.co/seleniumhqSuggestion importance[1-10]: 3
__
Why: The suggestion proposes a minor stylistic change to improve readability by replacing parentheses around a URL with a colon, which is a valid but low-impact improvement.
| PR 16174 (2025-08-13) |
[possible issue] Prevent invalid value reassignment
✅ Prevent invalid value reassignment
Avoid overwriting value with a non-dict message, which can lead to later .get() accesses failing. Only reassign value when message is a dict; otherwise keep value intact.
py/selenium/webdriver/remote/errorhandler.py [174-179]
if not isinstance(message, str):
- value = message
if isinstance(message, dict):
+ value = message
message = message.get("message")
else:
message = NoneSuggestion importance[1-10]: 7
__
Why: The suggestion correctly points out that value should only be reassigned if message is a dictionary, preventing potential AttributeError exceptions on subsequent .get() calls on value.
This wiki is not where you want to be! Visit the Wiki Home for more useful links
Getting Involved
Triaging Issues
Releasing Selenium
Ruby Development
Python Bindings
Ruby Bindings
WebDriverJs
This content is being evaluated for where it belongs
Architectural Overview
Automation Atoms
HtmlUnitDriver
Lift Style API
LoadableComponent
Logging
PageFactory
RemoteWebDriver
Xpath In WebDriver
Moved to Official Documentation
Bot Style Tests
Buck
Continuous Integration
Crazy Fun Build
Design Patterns
Desired Capabilities
Developer Tips
Domain Driven Design
Firefox Driver
Firefox Driver Internals
Focus Stealing On Linux
Frequently Asked Questions
Google Summer Of Code
Grid Platforms
History
Internet Explorer Driver
InternetExplorerDriver Internals
Next Steps
PageObjects
RemoteWebDriverServer
Roadmap
Scaling WebDriver
SeIDE Release Notes
Selenium Emulation
Selenium Grid 4
Selenium Help
Shipping Selenium 3
The Team
TLC Meetings
Untrusted SSL Certificates
WebDriver For Mobile Browsers
Writing New Drivers