titusfortner commented 1 month ago

User description

Description

Implements add and remove methods for websocket callers (there is definitely a better way to do this)
Deprecates BiDi#callbacks (is there another reason to keep this method?)
I think this is probably a replacement for LogInspector (which is not the high level API we want, and I don't think we need an intermediary implementation between the socket calls and the high level API)
Unlike LogInspector, this implementation doesn't do any type enforcement or structs or classes; return values are plain JSON. Do we want to add more structure and/or typing from that implementation into this?

Motivation and Context

Partial Ruby implementation of #13992

13964

Updates

Based on feedback:

using #object_id of the proc to return the id instead of tracking it separately
updated browser context implementation to avoid specifying browser context ids if they don't apply (which they don't for console logs)
Script class is going to implement things from both script and log domains. It makes sense to implement the domain specific functionality in separate, lower level classes and keep Script for the high level API, so I created LogHandler class for this.
Created a Struct subclass that will convert camel case JSON keys into snake case methods; using these Structs provides the user with a more straightforward access of declared parameters, without needing to maintain a lot of classes. If we dynamically generate classes in the future, it likely won't change the user's code
Deprecated Driver#script alias of Driver#exectue_script
Deprecated LogInspector - this should provide all of the required functionality of LogInspector

PR Type

Enhancement, Tests, Documentation

Description

Introduced LogHandler class for managing console and JavaScript log entries.
Deprecated BiDi#callbacks and LogInspector class.
Added add_callback and remove_callback methods to WebSocketConnection.
Created Script class to handle log entries and manage handlers.
Refactored subscribe and unsubscribe methods in Session class to use keyword arguments.
Added integration tests for Script class and BiDi error handling.
Updated type signatures for Driver and Script classes.

Changes walkthrough 📝

Relevant files

Enhancement

9 files

bidi.rb `Enhance BiDi with logging handlers and callback management` rb/lib/selenium/webdriver/bidi.rb Added `LogHandler` and `Struct` autoloads. Deprecated `BiDi#callbacks` method. Introduced `add_callback` and `remove_callback` methods.	+14/-0
log_handler.rb `Introduce LogHandler class for managing log entries` rb/lib/selenium/webdriver/bidi/log_handler.rb Created `LogHandler` class for managing log entries. Added methods to handle console and JavaScript log entries. Implemented subscription management for log entries.	+63/-0
log_inspector.rb `Deprecate LogInspector and update callback handling` rb/lib/selenium/webdriver/bidi/log_inspector.rb Deprecated `LogInspector` class. Updated method to use `add_callback` instead of direct callback manipulation.	+5/-1
session.rb `Refactor session subscription methods with keyword arguments` rb/lib/selenium/webdriver/bidi/session.rb Refactored `subscribe` and `unsubscribe` methods to use keyword arguments.	+6/-6
struct.rb `Add Struct class for camel to snake case conversion` rb/lib/selenium/webdriver/bidi/struct.rb Created `Struct` class to convert camel case JSON keys to snake case methods.	+40/-0
common.rb `Require Script class in common module` rb/lib/selenium/webdriver/common.rb - Added `require 'selenium/webdriver/common/script'`.	+1/-0
driver.rb `Deprecate Driver#script alias and add Script instance method` rb/lib/selenium/webdriver/common/driver.rb Deprecated `Driver#script` alias for `#execute_script`. Introduced `Driver#script` method for accessing `Script` instance.	+16/-6
script.rb `Add Script class for handling log entries` rb/lib/selenium/webdriver/common/script.rb Created `Script` class to handle console and JavaScript log entries.	+45/-0
websocket_connection.rb `Add callback management methods to WebSocketConnection` rb/lib/selenium/webdriver/common/websocket_connection.rb Added `add_callback` and `remove_callback` methods for managing WebSocket callbacks.	+9/-0

Tests

3 files

script_spec.rb `Add integration tests for Script class` rb/spec/integration/selenium/webdriver/bidi/script_spec.rb Added integration tests for `Script` class. Tested console message and JavaScript error handling.	+109/-0
bidi_spec.rb `Add BiDi error handling and deprecation tests` rb/spec/integration/selenium/webdriver/bidi_spec.rb Added test for BiDi error handling when not enabled. Added deprecation test for `BiDi#callbacks`.	+10/-23
driver_spec.rb `Add test for Driver#script method with deprecation warning` rb/spec/integration/selenium/webdriver/driver_spec.rb - Added test for `Driver#script` method with deprecation warning.	+10/-1

Documentation

2 files

driver.rbs `Update type signature for Driver class` rb/sig/lib/selenium/webdriver/common/driver.rbs - Added `@script` instance variable to type signature.	+1/-0
script.rbs `Add type signatures for Script class` rb/sig/selenium/web_driver/script.rbs - Added type signatures for `Script` class methods.	+20/-0

💡 PR-Agent usage: Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

codiumai-pr-agent-pro[bot] commented 1 month ago

PR Review 🔍

(Review updated until commit https://github.com/SeleniumHQ/selenium/commit/aeb7b85395982c4d2251f8be1caaa132fa2cdaf6)

⏱️ Estimated effort to review [1-5]	4, because the PR introduces significant changes across multiple files, including new classes and methods, deprecations, and modifications to existing functionalities. The complexity of the changes, especially around the new logging API and handling of callbacks, requires careful consideration to ensure compatibility and correctness.
🧪 Relevant tests	Yes
⚡ Possible issues	Possible Bug: The `remove_message_handler` method in `LogHandler` class might not properly handle the case where the `id` does not exist in the `@callbacks` hash, potentially leading to errors when trying to delete a non-existent entry.
🔒 Security concerns	No

Code feedback:

relevant file	rb/lib/selenium/webdriver/bidi/log_handler.rb
suggestion	Consider adding error handling in `remove_message_handler` to gracefully handle cases where the `id` provided does not exist in the `@callbacks` hash. This can prevent potential errors and improve the robustness of the method. [important]
relevant line	@bidi.remove_callback('log.entryAdded', id)

codiumai-pr-agent-pro[bot] commented 1 month ago

PR Code Suggestions ✨

Category	Suggestion	Score
Possible bug	Fix the `remove_callback` method to correctly use both `event` and `id` to fetch and remove the callback ___ The `remove_callback` method currently retrieves the callback using the `id` directly from `@track_callbacks`, which is incorrect. It should use the `event` and `id` to fetch the callback. [rb/lib/selenium/webdriver/bidi.rb [64-67]](https://github.com/SeleniumHQ/selenium/pull/14073/files#diff-b7660c7b53fec20ce95452b2afdbf306e9c36363e5bf8812863375b7bc96e42eR64-R67) ```diff def remove_callback(event, id) - call = @track_callbacks[id] + call = @track_callbacks[event][id] @callbacks[event].delete(call) + @track_callbacks[event].delete(id) end ``` Suggestion importance[1-10]: 10 Why: The suggestion correctly identifies and fixes a bug in the method implementation, ensuring the callback is properly accessed and removed using both `event` and `id`.	10
Consistency	Ensure `add_console_message_handler` and `add_javascript_error_handler` return the handler ID for consistency ___ The `add_console_message_handler` and `add_javascript_error_handler` methods should return the handler ID to be consistent with the `add_callback` method. [rb/lib/selenium/webdriver/common/script.rb [28-42]](https://github.com/SeleniumHQ/selenium/pull/14073/files#diff-72c27d09830dafa2065913fa74230f543e1d063b3f2bde3fe1c512192a569a4cR28-R42) ```diff +def add_console_message_handler + subscribe_log_entry unless @log_entry_subscribed + @bidi.add_callback('log.entryAdded') do \|params\| + yield(params) if params['type'] == 'console' + end +end +def add_javascript_error_handler + subscribe_log_entry unless @log_entry_subscribed + @bidi.add_callback('log.entryAdded') do \|params\| + yield(params) if params['type'] == 'javascript' + end +end ``` Suggestion importance[1-10]: 8 Why: This suggestion correctly identifies a consistency issue with the return values of handler methods, aligning them with similar methods. This is important for maintaining consistent API behavior.	8
Enhancement	Simplify the `script` method by using a ternary operator for better readability ___ The `script` method can be simplified by using a ternary operator to check if `args` is empty and return the appropriate result. [rb/lib/selenium/webdriver/common/driver.rb [107-113]](https://github.com/SeleniumHQ/selenium/pull/14073/files#diff-ecf89969518aec5e73ce3aff22dc10e88185534845a8a17228952c92f1498cddR107-R113) ```diff def script(args) - if args.any? - execute_script(args) - else - @script \|\|= WebDriver::Script.new(bridge) - end + args.any? ? execute_script(args) : (@script \|\|= WebDriver::Script.new(bridge)) end ``` Suggestion importance[1-10]: 6* Why: The suggestion improves readability by simplifying the method with a ternary operator. However, it's a minor enhancement rather than a critical change.	6
Maintainability	Correct the typo in the test method name for better readability ___ There is a typo in the test method name `logs removes console message hander`. It should be corrected to `logs removes console message handler`. [rb/spec/integration/selenium/webdriver/bidi/script_spec.rb [67]](https://github.com/SeleniumHQ/selenium/pull/14073/files#diff-511fb8dd07c92b6e7a8767aafc8f3008f7fb18799fd027b9ee9b4e45d9cbea33R67-R67) ```diff -it 'logs removes console message hander' do +it 'logs removes console message handler' do ``` Suggestion importance[1-10]: 4 Why: Correcting typos improves code maintainability and readability, but it's a relatively minor issue.	4

codiumai-pr-agent-pro[bot] commented 1 month ago

CI Failure Feedback 🧐

(Checks updated until commit https://github.com/SeleniumHQ/selenium/commit/4252b0090a5fc926023e17b8b51b752cb6be5ec0)

**Action:** Ruby / Local Tests (firefox, windows) / Local Tests (firefox, windows)

**Failed stage:** [Run Bazel](https://github.com/SeleniumHQ/selenium/actions/runs/9386108330/job/25846231393) [❌]

**Failed test name:** //rb/spec/integration/selenium/webdriver:devtools-firefox

**Failure summary:** The action failed due to the following reasons:

The protoc.exe command failed to execute successfully, causing a critical build step to fail.

Specifically, the error occurred while generating the proto_library for the target
@@io_bazel_rules_closure//java/io/bazel/rules/closure:build_info_proto.

The failure of this build step resulted in the overall build not completing successfully.

Consequently, the test //rb/spec/integration/selenium/webdriver:devtools-firefox failed to build,
leading to the failure of the action.

Relevant error logs:

```yaml 1: ##[group]Operating System 2: Microsoft Windows Server 2022 ... 353: ##[group]Restore cache for external-protobuf~ 354: Cache Size: ~7 MB (6846456 B) 355: [command]"C:\Program Files\Git\usr\bin\tar.exe" -xf D:/a/_temp/0fb72dd8-d190-43e5-b123-7d8f3e791412/cache.tzst -P -C D:/a/selenium/selenium --force-local --use-compress-program "zstd -d" 356: Received 5073488 of 5073488 (100.0%), 4.8 MBs/sec 357: Cache restored successfully 358: Successfully restored cache from setup-bazel-2-win32-external-protobuf~-9914fdedced4b237d23a421264c215c7ce93f92c099c523923a35f2748b6e88d 359: ##[endgroup] 360: ##[group]Restore cache for external-rules_buf~~ext~rules_buf_toolchains 361: Failed to restore external-rules_buf~~ext~rules_buf_toolchains cache ... 664: [32m[1,066 / 1,864][0m Compiling src/google/protobuf/compiler/command_line_interface.cc [for tool]; 5s local, disk-cache ... (4 actions, 3 running) 665: [32m[1,074 / 1,864][0m Compiling src/google/protobuf/compiler/command_line_interface.cc [for tool]; 6s local, disk-cache ... (2 actions, 1 running) 666: [32m[1,076 / 1,864][0m [Prepa] RunBinary rb/lib/selenium/devtools/v124.rb ... (4 actions, 1 running) 667: [32mINFO: [0mFrom Installing external/rules_ruby~~ruby~bundle/rb/vendor/cache/bundler-2.5.10.gem (@@rules_ruby~~ruby~bundle//:bundler-2.5.10): 668: Successfully installed bundler-2.5.10 669: 1 gem installed 670: [32m[1,077 / 1,864][0m Linking external/protobuf~/protoc.exe [for tool]; 1s local, disk-cache ... (4 actions, 1 running) 671: [32m[1,078 / 1,864][0m RunBinary rb/lib/selenium/devtools/v85.rb; 1s disk-cache ... (4 actions, 2 running) 672: [31m[1mERROR: [0mD:/_bazel/external/io_bazel_rules_closure/java/io/bazel/rules/closure/BUILD:64:14: Generating proto_library @@io_bazel_rules_closure//java/io/bazel/rules/closure:build_info_proto [for tool] failed: (Exit -1073741819): protoc.exe failed: error executing GenProto command (from target @@io_bazel_rules_closure//java/io/bazel/rules/closure:build_info_proto) 673: cd /d D:/_bazel/execroot/_main 674: SET PATH=C:\Program Files\Git\bin;C:\Program Files\Git\usr\bin;C:\Windows;C:\Windows\System32;C:\Windows\System32\WindowsPowerShell\v1.0 675: bazel-out\x64_windows-opt-exec-ST-13d3ddad9198\bin\external\protobuf~\protoc.exe --java_out=bazel-out/x64_windows-opt-exec-ST-13d3ddad9198/bin/external/io_bazel_rules_closure/java/io/bazel/rules/closure/build_info_proto-speed-src.jar -Iexternal/io_bazel_rules_closure -I. external/io_bazel_rules_closure/java/io/bazel/rules/closure/build_info.proto 676: # Configuration: 3f62d58e1724b33c4c5a838ec5dd37be29e6f58351fe7900d780cb5e8258345c 677: # Execution platform: @@local_config_platform//:host 678: [32m[1,082 / 1,864][0m 1 / 27 tests, [31m[1m1 failed[0m;[0m checking cached actions 679: [32mINFO: [0mElapsed time: 266.804s, Critical Path: 16.05s 680: [32mINFO: [0m1082 processes: 33 disk cache hit, 872 internal, 177 local. 681: [31m[1mERROR: [0mBuild did NOT complete successfully 682: //rb/spec/integration/selenium/webdriver:action_builder-firefox [0m[31m[1mNO STATUS[0m 683: //rb/spec/integration/selenium/webdriver:bidi-firefox [0m[31m[1mNO STATUS[0m 684: //rb/spec/integration/selenium/webdriver:driver-firefox [0m[31m[1mNO STATUS[0m 685: //rb/spec/integration/selenium/webdriver:element-firefox [0m[31m[1mNO STATUS[0m 686: //rb/spec/integration/selenium/webdriver:error-firefox [0m[31m[1mNO STATUS[0m ... 700: //rb/spec/integration/selenium/webdriver/bidi:browsing_context-firefox [0m[31m[1mNO STATUS[0m 701: //rb/spec/integration/selenium/webdriver/bidi:log_inspector-firefox [0m[31m[1mNO STATUS[0m 702: //rb/spec/integration/selenium/webdriver/bidi:script-firefox [0m[31m[1mNO STATUS[0m 703: //rb/spec/integration/selenium/webdriver/firefox:driver-firefox [0m[31m[1mNO STATUS[0m 704: //rb/spec/integration/selenium/webdriver/firefox:profile-firefox [0m[31m[1mNO STATUS[0m 705: //rb/spec/integration/selenium/webdriver/firefox:service-firefox [0m[31m[1mNO STATUS[0m 706: //rb/spec/integration/selenium/webdriver/remote:driver-firefox [0m[31m[1mNO STATUS[0m 707: //rb/spec/integration/selenium/webdriver/remote:element-firefox [0m[31m[1mNO STATUS[0m 708: //rb/spec/integration/selenium/webdriver:devtools-firefox [0m[31m[1mFAILED TO BUILD[0m 709: Executed 0 out of 27 tests: [0m[31m[1m1 fails to build[0m and [0m[35m26 were skipped[0m. 710: [0m 711: ##[error]Process completed with exit code 1. ```

✨ CI feedback usage guide:

The CI feedback tool (`/checks)` automatically triggers when a PR has a failed check. The tool analyzes the failed checks and provides several feedbacks: - Failed stage - Failed test name - Failure summary - Relevant error logs In addition to being automatically triggered, the tool can also be invoked manually by commenting on a PR: ``` /checks "https://github.com/{repo_name}/actions/runs/{run_number}/job/{job_number}" ``` where `{repo_name}` is the name of the repository, `{run_number}` is the run number of the failed check, and `{job_number}` is the job number of the failed check. #### Configuration options - `enable_auto_checks_feedback` - if set to true, the tool will automatically provide feedback when a check is failed. Default is true. - `excluded_checks_list` - a list of checks to exclude from the feedback, for example: ["check1", "check2"]. Default is an empty list. - `enable_help_text` - if set to true, the tool will provide a help message with the feedback. Default is true. - `persistent_comment` - if set to true, the tool will overwrite a previous checks comment with the new feedback. Default is true. - `final_update_message` - if `persistent_comment` is true and updating a previous checks message, the tool will also create a new message: "Persistent checks updated to latest commit". Default is true. See more information about the `checks` tool in the [docs](https://pr-agent-docs.codium.ai/tools/ci_feedback/).

titusfortner commented 4 weeks ago

/help

codiumai-pr-agent-pro[bot] commented 4 weeks ago

PR Agent Walkthrough 🤖

Welcome to the PR Agent, an AI-powered tool for automated pull request analysis, feedback, suggestions and more.

Here is a list of tools you can use to interact with the PR Agent:

Tool	Description	Trigger Interactively :gem:
[DESCRIBE](https://pr-agent-docs.codium.ai/tools/describe/)	Generates PR description - title, type, summary, code walkthrough and labels	- [ ] Run
[REVIEW](https://pr-agent-docs.codium.ai/tools/review/)	Adjustable feedback about the PR, possible issues, security concerns, review effort and more	- [ ] Run
[IMPROVE](https://pr-agent-docs.codium.ai/tools/improve/)	Code suggestions for improving the PR	- [x] Run
[UPDATE CHANGELOG](https://pr-agent-docs.codium.ai/tools/update_changelog/)	Automatically updates the changelog	- [ ] Run
[ADD DOCS](https://pr-agent-docs.codium.ai/tools/documentation/) 💎	Generates documentation to methods/functions/classes that changed in the PR	- [ ] Run
[TEST](https://pr-agent-docs.codium.ai/tools/test/) 💎	Generates unit tests for a specific component, based on the PR code change	- [ ] Run
[IMPROVE COMPONENT](https://pr-agent-docs.codium.ai/tools/improve_component/) 💎	Code suggestions for a specific component that changed in the PR	- [ ] Run
[ANALYZE](https://pr-agent-docs.codium.ai/tools/analyze/) 💎	Identifies code components that changed in the PR, and enables to interactively generate tests, docs, and code suggestions for each component	- [ ] Run
[ASK](https://pr-agent-docs.codium.ai/tools/ask/)	Answering free-text questions about the PR	[*]
[GENERATE CUSTOM LABELS](https://pr-agent-docs.codium.ai/tools/custom_labels/) 💎	Generates custom labels for the PR, based on specific guidelines defined by the user	[*]
[CI FEEDBACK](https://pr-agent-docs.codium.ai/tools/ci_feedback/) 💎	Generates feedback and analysis for a failed CI job	[*]
[CUSTOM PROMPT](https://pr-agent-docs.codium.ai/tools/custom_prompt/) 💎	Generates custom suggestions for improving the PR code, derived only from a specific guidelines prompt defined by the user	[*]
[SIMILAR ISSUE](https://pr-agent-docs.codium.ai/tools/similar_issues/)	Automatically retrieves and presents similar issues	[*]

(1) Note that each tool be triggered automatically when a new PR is opened, or called manually by commenting on a PR.

(2) Tools marked with [*] require additional parameters to be passed. For example, to invoke the /ask tool, you need to comment on a PR: /ask "<question content>". See the relevant documentation for each tool for more details.

codiumai-pr-agent-pro[bot] commented 4 weeks ago

Persistent review updated to latest commit https://github.com/SeleniumHQ/selenium/commit/aeb7b85395982c4d2251f8be1caaa132fa2cdaf6

codiumai-pr-agent-pro[bot] commented 4 weeks ago

PR Description updated to latest commit (https://github.com/SeleniumHQ/selenium/commit/aeb7b85395982c4d2251f8be1caaa132fa2cdaf6)

[ ] Copy walkthrough table to "Files Changed" Tab

codiumai-pr-agent-pro[bot] commented 4 weeks ago

PR Code Suggestions ✨

Category	Suggestion	Score
Enhancement	Ensure `add_message_handler` returns the ID of the callback handler it adds ___ The `add_message_handler` method should return the ID of the callback handler it adds. This will make it consistent with the `WebSocketConnection#add_callback` method and allow the caller to remove the handler later. [rb/lib/selenium/webdriver/bidi/log_handler.rb [33-41]](https://github.com/SeleniumHQ/selenium/pull/14073/files#diff-255281b194a6769984ac959abce6e066d00fbfc11e43f8b21363695d0dd652b8R33-R41) ```diff +def add_message_handler(type) + subscribe_log_entry unless @log_entry_subscribed + @bidi.add_callback('log.entryAdded') do \|params\| + if params['type'] == type + log_entry_klass = type == 'console' ? ConsoleLogEntry : JavaScriptLogEntry + yield(log_entry_klass.new(params)) + end + end +end - ``` Suggestion importance[1-10]: 10** Why: The suggestion correctly identifies a functional inconsistency and proposes a change that aligns with existing patterns in the codebase, enhancing usability and consistency.	10
Possible issue	Add a check to ensure the handler ID exists before attempting to remove it ___ Add a check in the `remove_message_handler` method to ensure the handler ID exists before attempting to remove it. This will prevent potential errors if an invalid ID is passed. [rb/lib/selenium/webdriver/bidi/log_handler.rb [44-47]](https://github.com/SeleniumHQ/selenium/pull/14073/files#diff-255281b194a6769984ac959abce6e066d00fbfc11e43f8b21363695d0dd652b8R44-R47) ```diff def remove_message_handler(id) - @bidi.remove_callback('log.entryAdded', id) - unsubscribe_log_entry if @bidi.callbacks['log.entryAdded'].empty? + if @bidi.callbacks['log.entryAdded'].any? { \|callback\| callback.object_id == id } + @bidi.remove_callback('log.entryAdded', id) + unsubscribe_log_entry if @bidi.callbacks['log.entryAdded'].empty? + else + raise ArgumentError, "Handler ID not found" + end end ``` Suggestion importance[1-10]: 8 Why: Adding a check for the existence of a handler ID before removal is crucial to prevent runtime errors and ensures robustness in method execution.	8
Best practice	Convert `camel_to_snake` to a class method for better reusability ___ The `camel_to_snake` method should be defined as a class method since it does not depend on instance-specific data. This will make it more reusable and consistent with its usage. [rb/lib/selenium/webdriver/bidi/struct.rb [34-36]](https://github.com/SeleniumHQ/selenium/pull/14073/files#diff-04c05a628e46aa69c7d088546e665bcdd069b1869bc82d7e0466d4915624f3b7R34-R36) ```diff -def camel_to_snake(camel_str) +def self.camel_to_snake(camel_str) camel_str.gsub(/([A-Z])/, '_\1').downcase end ``` Suggestion importance[1-10]: 7 Why: Converting `camel_to_snake` to a class method is a good practice as it does not rely on instance state, making the method more reusable.	7
Maintainability	Extract deprecation warning logic into a separate private method for better readability ___ To improve readability and maintainability, consider extracting the deprecation warning logic in the `callbacks` method into a separate private method. [rb/lib/selenium/webdriver/bidi.rb [39-40]](https://github.com/SeleniumHQ/selenium/pull/14073/files#diff-b7660c7b53fec20ce95452b2afdbf306e9c36363e5bf8812863375b7bc96e42eR39-R40) ```diff -WebDriver.logger.deprecate('`BiDi#callbacks << event` to add a callback to the list of callbacks', - 'BiDi#add_callback(event, &block)', id: :callbacks) +log_deprecation_warning +private + +def log_deprecation_warning + WebDriver.logger.deprecate('`BiDi#callbacks << event` to add a callback to the list of callbacks', + 'BiDi#add_callback(event, &block)', id: :callbacks) +end + ``` Suggestion importance[1-10]: 6 Why: Extracting the deprecation logic into a separate method improves the readability and maintainability of the code, although it's not a critical change.	6

titusfortner commented 4 weeks ago

I have some more tweaks for this, but does it look good enough for now?

codiumai-pr-agent-pro[bot] commented 4 weeks ago

PR Code Suggestions ✨

Category	Suggestion	Score
Enhancement	Return the handler ID from the `add_message_handler` method to allow users to remove the handler later ___ The `add_message_handler` method should return the handler ID to allow users to remove the handler later. This will make the method consistent with `remove_message_handler` which requires an ID. [rb/lib/selenium/webdriver/bidi/log_handler.rb [33-41]](https://github.com/SeleniumHQ/selenium/pull/14073/files#diff-255281b194a6769984ac959abce6e066d00fbfc11e43f8b21363695d0dd652b8R33-R41) ```diff +def add_message_handler(type) + subscribe_log_entry unless @log_entry_subscribed + @bidi.add_callback('log.entryAdded') do \|params\| + if params['type'] == type + log_entry_klass = type == 'console' ? ConsoleLogEntry : JavaScriptLogEntry + yield(log_entry_klass.new(params)) + end + end +end - ``` Suggestion importance[1-10]: 8** Why: This is a crucial improvement for consistency and usability, allowing users to manage handlers effectively.	8
Enhancement	Return a boolean indicating whether the callback was successfully removed in the `remove_callback` method ___ The `remove_callback` method should return a boolean indicating whether the callback was successfully removed to provide feedback to the caller. [rb/lib/selenium/webdriver/common/websocket_connection.rb [60-64]](https://github.com/SeleniumHQ/selenium/pull/14073/files#diff-6675f0e8e985c3acbf5be1124a44431bc20e31f11f7c8054edc572a0e5de66daR60-R64) ```diff def remove_callback(event, id) - return if callbacks[event].reject! { \|callback\| callback.object_id == id } + removed = callbacks[event].reject! { \|callback\| callback.object_id == id } + return true if removed ids = callbacks[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: This suggestion enhances the method by providing clear feedback on the operation's success, which is useful for error handling.	7
Possible bug	Handle the case where the event does not exist in the callbacks hash in the `remove_message_handler` method ___ The `remove_message_handler` method should handle the case where the event does not exist in the callbacks hash to avoid potential errors. [rb/lib/selenium/webdriver/bidi/log_handler.rb [44-47]](https://github.com/SeleniumHQ/selenium/pull/14073/files#diff-255281b194a6769984ac959abce6e066d00fbfc11e43f8b21363695d0dd652b8R44-R47) ```diff def remove_message_handler(id) + return unless @bidi.callbacks.key?('log.entryAdded') @bidi.remove_callback('log.entryAdded', id) unsubscribe_log_entry if @log_entry_subscribed && @bidi.callbacks['log.entryAdded'].empty? end ``` Suggestion importance[1-10]: 7 Why: This suggestion correctly identifies a potential error scenario and improves robustness by handling missing events.	7
Best practice	Define `camel_to_snake` as a class method since it does not depend on instance state ___ The `camel_to_snake` method should be defined as a class method since it does not depend on instance state and can be reused without instantiating the class. [rb/lib/selenium/webdriver/bidi/struct.rb [34-36]](https://github.com/SeleniumHQ/selenium/pull/14073/files#diff-04c05a628e46aa69c7d088546e665bcdd069b1869bc82d7e0466d4915624f3b7R34-R36) ```diff -def camel_to_snake(camel_str) +def self.camel_to_snake(camel_str) camel_str.gsub(/([A-Z])/, '_\1').downcase end ``` Suggestion importance[1-10]: 6 Why: Converting `camel_to_snake` to a class method is a good practice for utility methods that don't rely on instance state, enhancing reusability.	6

SeleniumHQ / selenium

[rb] Implement High Level Logging API with BiDi #14073

User description

Description

Motivation and Context

13964

Updates

PR Type

Description

Changes walkthrough 📝

PR Review 🔍

(Review updated until commit https://github.com/SeleniumHQ/selenium/commit/aeb7b85395982c4d2251f8be1caaa132fa2cdaf6)

PR Code Suggestions ✨

CI Failure Feedback 🧐

(Checks updated until commit https://github.com/SeleniumHQ/selenium/commit/4252b0090a5fc926023e17b8b51b752cb6be5ec0)

PR Agent Walkthrough 🤖

PR Code Suggestions ✨

PR Code Suggestions ✨