[js] Close BiDi websocket connection

[pujagani]

User description

Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

Motivation and Context

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist

• I have read the contributing document.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added tests to cover my changes.
• All new and existing tests passed.

---

PR Type

enhancement

---

Description

• Renamed the websocket connection variable from this._wsConnection to this._cdpWsConnection to improve clarity and consistency.
• Added functionality to close the BiDi websocket connection to prevent node processes from hanging.
• Modified the getBidi method to ensure the BiDi connection is initialized only once.
• Updated all websocket event handlers to use the newly renamed this._cdpWsConnection variable.

---

Changes walkthrough 📝

	Relevant files
Enhancement	webdriver.js Enhance websocket connection management and naming              javascript/node/selenium-webdriver/lib/webdriver.js Renamed this._wsConnection to this._cdpWsConnection for clarity. Added logic to close the BiDi websocket connection. Ensured BiDi connection is initialized only once. Updated websocket event handlers to use the new variable name. +23/-15

---

💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

[qodo-merge-pro]

PR Reviewer Guide 🔍

(Review updated until commit 243c068)

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Key issues to review Potential Memory Leak The getBidi() method creates a new BIDI connection if it doesn't exist, but there's no mechanism to close this connection when it's no longer needed. Error Handling The error handling in the WebSocket connection setup doesn't provide specific error information, which might make debugging issues harder.

[qodo-merge-pro]

PR Code Suggestions ✨

Latest suggestions up to 243c068

Category	Suggestion	Score
Best practice	Check if the WebSocket connection is open before closing it Consider using a more specific check for the existence of this._cdpWsConnection, such as this._cdpWsConnection && this._cdpWsConnection.readyState === WebSocket.OPEN, to ensure the connection is actually open before attempting to close it. javascript/node/selenium-webdriver/lib/webdriver.js [792-794] -if (this._cdpWsConnection !== undefined) { +if (this._cdpWsConnection && this._cdpWsConnection.readyState === WebSocket.OPEN) { this._cdpWsConnection.close() } Apply this suggestion Suggestion importance[1-10]: 9 Why: This suggestion improves the robustness of the code by ensuring that the WebSocket connection is actually open before attempting to close it, which prevents potential errors and aligns with best practices.	9
Possible issue	Add a check for the existence of a required value before using it Consider adding a check to ensure that WebSocketUrl is not undefined before using it, to prevent potential runtime errors. javascript/node/selenium-webdriver/lib/webdriver.js [1298-1299] let WebSocketUrl = caps['map_'].get('webSocketUrl') -this._bidiConnection = new BIDI(WebSocketUrl.replace('localhost', '127.0.0.1')) +if (WebSocketUrl) { + this._bidiConnection = new BIDI(WebSocketUrl.replace('localhost', '127.0.0.1')) +} else { + throw new Error('WebSocket URL is not available in capabilities') +} Apply this suggestion Suggestion importance[1-10]: 9 Why: This suggestion is crucial as it prevents runtime errors by ensuring that a necessary value is present before use, which is a fundamental practice in defensive programming.	9
Error handling	Add error handling when closing connections Consider adding error handling when closing the BiDi connection to prevent potential exceptions from interrupting the quit process. javascript/node/selenium-webdriver/lib/webdriver.js [797-799] if (this._bidiConnection !== undefined) { - this._bidiConnection.close() + try { + this._bidiConnection.close() + } catch (error) { + console.error('Error closing BiDi connection:', error) + } } Apply this suggestion Suggestion importance[1-10]: 8 Why: Adding error handling when closing connections is a good practice to prevent unexpected exceptions from disrupting the application flow, especially during critical operations like quitting.	8
Maintainability	Use consistent naming conventions for variables Consider using a more descriptive variable name for WebSocketUrl to follow JavaScript naming conventions and improve code readability. javascript/node/selenium-webdriver/lib/webdriver.js [1298-1299] -let WebSocketUrl = caps['map_'].get('webSocketUrl') -this._bidiConnection = new BIDI(WebSocketUrl.replace('localhost', '127.0.0.1')) +let webSocketUrl = caps['map_'].get('webSocketUrl') +this._bidiConnection = new BIDI(webSocketUrl.replace('localhost', '127.0.0.1')) Apply this suggestion Suggestion importance[1-10]: 7 Why: Using camelCase for variable names in JavaScript is a standard convention that enhances code readability and maintainability, making this a useful suggestion for consistency.	7

💡 Need additional feedback ? start a PR chat

---

Previous suggestions

Suggestions up to commit 878dbab

Category	Suggestion	Score
Best practice	Check if the WebSocket connection is open before closing it Consider using a more specific check for the existence of this._cdpWsConnection, such as this._cdpWsConnection && this._cdpWsConnection.readyState === WebSocket.OPEN, to ensure the connection is actually open before attempting to close it. javascript/node/selenium-webdriver/lib/webdriver.js [792-794] -if (this._cdpWsConnection !== undefined) { +if (this._cdpWsConnection && this._cdpWsConnection.readyState === WebSocket.OPEN) { this._cdpWsConnection.close() } Suggestion importance[1-10]: 9 Why: This suggestion improves the robustness of the code by ensuring that the WebSocket connection is actually open before attempting to close it, which can prevent potential runtime errors.	9
Error handling	Add error handling when closing connections Consider adding error handling when closing the BiDi connection to prevent potential exceptions from interrupting the quit process. javascript/node/selenium-webdriver/lib/webdriver.js [797-799] if (this._bidiConnection !== undefined) { - this._bidiConnection.close() + try { + this._bidiConnection.close() + } catch (error) { + console.warn('Error closing BiDi connection:', error) + } } Suggestion importance[1-10]: 8 Why: Adding error handling when closing the BiDi connection is a good practice to prevent exceptions from interrupting the quit process, enhancing the stability of the application.	8
	Add a check for the existence of required capabilities Consider adding a check for the existence of the 'webSocketUrl' capability before accessing it to prevent potential errors if the capability is not present. javascript/node/selenium-webdriver/lib/webdriver.js [1297-1299] const caps = await this.getCapabilities() let WebSocketUrl = caps['map_'].get('webSocketUrl') +if (!WebSocketUrl) { + throw new Error('WebSocket URL not found in capabilities') +} this._bidiConnection = new BIDI(WebSocketUrl.replace('localhost', '127.0.0.1')) Suggestion importance[1-10]: 8 Why: Adding a check for the existence of the 'webSocketUrl' capability prevents potential errors if the capability is not present, which is crucial for error handling and application stability.	8
Enhancement	Use a more robust method for URL manipulation Consider using a more robust method to replace 'localhost' with '127.0.0.1', such as using a URL object, to handle potential edge cases in the WebSocket URL. javascript/node/selenium-webdriver/lib/webdriver.js [1249] -this._cdpWsConnection = new WebSocket(this._wsUrl.replace('localhost', '127.0.0.1')) +const url = new URL(this._wsUrl); +url.hostname = url.hostname === 'localhost' ? '127.0.0.1' : url.hostname; +this._cdpWsConnection = new WebSocket(url.toString()); Suggestion importance[1-10]: 7 Why: Using a URL object for hostname replacement is a more robust method that can handle potential edge cases, improving the reliability of the code.	7

Suggestions up to commit a37a727

Category	Suggestion	Score
Enhancement	Use optional chaining for safer BiDi connection closure Consider using optional chaining (?.) when closing the BiDi connection to handle cases where this._bidi might be undefined, preventing potential errors. javascript/node/selenium-webdriver/lib/webdriver.js [796-798] -if (this._bidi !== undefined) { - this._bidi.close() -} +this._bidi?.close() Suggestion importance[1-10]: 9 Why: The use of optional chaining (?.) is a safer and more concise way to handle potential undefined values, preventing runtime errors. This suggestion correctly addresses a potential issue in the code.	9
Best practice	Improve variable naming convention for better code readability Consider using a more descriptive variable name instead of WebSocketUrl to improve code readability and follow JavaScript naming conventions for local variables. javascript/node/selenium-webdriver/lib/webdriver.js [1297] -let WebSocketUrl = caps['map_'].get('webSocketUrl') +let webSocketUrl = caps['map_'].get('webSocketUrl') Suggestion importance[1-10]: 7 Why: Changing WebSocketUrl to webSocketUrl aligns with JavaScript naming conventions for local variables, improving code readability. While beneficial, this is a minor improvement.	7

[qodo-merge-pro]

Persistent review updated to latest commit 878dbab

[qodo-merge-pro]

Persistent review updated to latest commit 243c068