add templating capabilities #3
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes introduce a Unix socket server to the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant UploadsCli
participant RequestInterceptor
Client->>UploadsCli: set_template(key, filename)
UploadsCli->>RequestInterceptor: send message (set, key, template content)
RequestInterceptor->>RequestInterceptor: store template
Client->>UploadsCli: remove_template(key)
UploadsCli->>RequestInterceptor: send message (remove, key)
RequestInterceptor->>RequestInterceptor: remove template
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (3)
lib/uploads_cli.rb (1)
Line range hint
1-103: Consider adding documentation and separating socket communication concerns.The new template management functionality aligns well with the PR objective. However, there are a few areas that could be improved:
- Add documentation for the new methods
set_templateandremove_template. This will help other developers understand their purpose and usage.- Consider extracting the socket communication logic into a separate module. This would improve the separation of concerns and make the
UploadsCLIclass more focused on its primary responsibilities.Here's a suggestion for documenting the new methods:
# Set a template for the given key using the content of the specified file # # @param key [String] The key to associate with the template # @param filename [String] The path to the file containing the template content # @raise [Errno::ENOENT] If the specified file does not exist def set_template(key, filename) # ... implementation ... end # Remove the template associated with the given key # # @param key [String] The key of the template to remove def remove_template(key) # ... implementation ... endFor socket communication, consider creating a
SocketCommunicatormodule:module SocketCommunicator def send_socket_message(message) # ... implementation ... end # ... other socket-related methods ... end class UploadsCLI < Thor include SocketCommunicator # ... rest of the class ... endThis approach would make the code more modular and easier to maintain.
🧰 Tools
🪛 rubocop
[convention] 69-88: Method has too many lines. [14/10]
(Metrics/MethodLength)
lib/request_interceptor.rb (2)
83-83: Use safe navigation operatorInstead of checking if
@serverexists before callingclose, you can use the safe navigation operator&.for cleaner code.Apply this diff:
-@server.close if @server +@server&.close🧰 Tools
🪛 rubocop
[convention] 83-83: Use safe navigation (
&.) instead of checking if an object exists before calling the method.(Style/SafeNavigation)
88-88: Clarify assignment in condition by adding parenthesesIn the
whileloop, the assignment within the condition can be misread as a comparison. Wrapping the assignment in parentheses clarifies the intent and avoids potential warnings.Apply this diff:
-while message = conn.gets +while (message = conn.gets)🧰 Tools
🪛 rubocop
[warning] 88-88: Use
==if you meant to do a comparison or wrap the expression in parentheses to indicate you meant to assign in a condition.(Lint/AssignmentInCondition)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- lib/request_interceptor.rb (3 hunks)
- lib/uploads_cli.rb (2 hunks)
🧰 Additional context used
🪛 rubocop
lib/request_interceptor.rb
[convention] 28-58: Assignment Branch Condition size for call is too high. [<10, 20, 5> 22.91/17]
(Metrics/AbcSize)
[convention] 28-58: Method has too many lines. [19/10]
(Metrics/MethodLength)
[convention] 62-85: Assignment Branch Condition size for start_unix_socket_server is too high. [<5, 16, 4> 17.23/17]
(Metrics/AbcSize)
[convention] 62-85: Method has too many lines. [19/10]
(Metrics/MethodLength)
[convention] 83-83: Use safe navigation (
&.) instead of checking if an object exists before calling the method.(Style/SafeNavigation)
[warning] 88-88: Use
==if you meant to do a comparison or wrap the expression in parentheses to indicate you meant to assign in a condition.(Lint/AssignmentInCondition)
[convention] 98-120: Assignment Branch Condition size for process_message is too high. [<6, 17, 3> 18.28/17]
(Metrics/AbcSize)
[convention] 98-120: Method has too many lines. [21/10]
(Metrics/MethodLength)
lib/uploads_cli.rb
[convention] 69-88: Method has too many lines. [14/10]
(Metrics/MethodLength)
| @@ -26,6 +26,7 @@ def initialize(*args) | |||
| @client_uuid = current_client_uuid | |||
| @file_uploader = FileUploader.new(@logger, @client_uuid) | |||
| @command_executor = CommandExecutor.new(@logger, @client_uuid) | |||
| @socket_path = '/tmp/hawksi.sock' | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving socket path configuration and error handling.
The socket path is currently hardcoded, which might limit flexibility across different environments. Consider the following improvements:
- Make the socket path configurable, possibly through an environment variable or a configuration file.
- Add a check to ensure the socket file exists and is accessible when initializing the class.
Here's a suggested implementation:
def initialize(*args)
super
# ... existing code ...
@socket_path = ENV['HAWKSI_SOCKET_PATH'] || '/tmp/hawksi.sock'
validate_socket_path
end
private
def validate_socket_path
unless File.exist?(@socket_path) && File.socket?(@socket_path)
raise "Socket file not found or invalid: #{@socket_path}"
end
endThis change allows for configuration via an environment variable and includes a basic validation of the socket file.
| desc 'set_template KEY FILENAME', 'Set a template for the given key using the content of the specified file' | ||
| option :base_dir, type: :string, | ||
| desc: 'Base directory for storing intercepted data. Defaults to ./tmp/intercepted_data' | ||
| def set_template(key, filename) | ||
| set_base_dir | ||
| unless File.exist?(filename) | ||
| @logger.error "File not found: #{filename}" | ||
| return | ||
| end | ||
|
|
||
| # Read the template content from the file | ||
| template_content = File.read(filename) | ||
|
|
||
| message = { | ||
| action: 'set_template', | ||
| key: key, | ||
| template: template_content | ||
| } | ||
| # Send the message to the /tmp/hawksi.sock socket | ||
| socket = UNIXSocket.new(@socket_path) | ||
| socket.write(message.to_json) | ||
| socket.close | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor set_template method for improved error handling and readability.
The set_template method could benefit from some refactoring to improve error handling, reduce method length, and ensure proper socket communication. Consider the following suggestions:
- Wrap the socket communication in a begin-rescue block to handle potential errors.
- Ensure the socket is always closed, even in case of an error.
- Extract the file reading and socket communication into separate private methods to reduce the main method's length.
Here's a suggested refactoring:
def set_template(key, filename)
set_base_dir
template_content = read_template_file(filename)
send_template_message(key, template_content) if template_content
end
private
def read_template_file(filename)
File.read(filename)
rescue Errno::ENOENT
@logger.error "File not found: #{filename}"
nil
end
def send_template_message(key, template_content)
message = {
action: 'set_template',
key: key,
template: template_content
}
send_socket_message(message)
end
def send_socket_message(message)
socket = UNIXSocket.new(@socket_path)
socket.write(message.to_json)
rescue StandardError => e
@logger.error "Error sending message to socket: #{e.message}"
ensure
socket&.close
endThis refactoring improves error handling, ensures the socket is always closed, and reduces the main method's length by extracting functionality into separate methods.
🧰 Tools
🪛 rubocop
[convention] 69-88: Method has too many lines. [14/10]
(Metrics/MethodLength)
| desc 'remove_template KEY', 'Remove the template for the given key' | ||
| option :base_dir, type: :string, | ||
| desc: 'Base directory for storing intercepted data. Defaults to ./tmp/intercepted_data' | ||
| def remove_template(key) | ||
| set_base_dir | ||
| message = { | ||
| action: 'remove_template', | ||
| key: key | ||
| } | ||
| socket = UNIXSocket.new(@socket_path) | ||
| socket.write(message.to_json) | ||
| socket.close | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve remove_template method with error handling and code reuse.
The remove_template method could benefit from improved error handling and code reuse. Consider the following suggestions:
- Reuse the
send_socket_messagemethod suggested in the previous comment for consistent error handling and socket management. - Keep the method focused on its specific task by extracting the message creation.
Here's a suggested refactoring:
def remove_template(key)
set_base_dir
message = create_remove_template_message(key)
send_socket_message(message)
end
private
def create_remove_template_message(key)
{
action: 'remove_template',
key: key
}
end
# Use the send_socket_message method from the previous commentThis refactoring improves error handling, ensures consistent socket management, and promotes code reuse between set_template and remove_template methods.
| def generate_request_key(request) | ||
| "#{request.request_method}_#{request.path}" | ||
| end |
There was a problem hiding this comment.
Include query parameters in request key to ensure uniqueness
The generate_request_key method uses request.request_method and request.path, which might not be unique if the same path is used with different query parameters. To avoid key collisions, consider including the query string in the request key.
Apply this diff to include query parameters:
def generate_request_key(request)
- "#{request.request_method}_#{request.path}"
+ "#{request.request_method}_#{request.fullpath}"
end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def generate_request_key(request) | |
| "#{request.request_method}_#{request.path}" | |
| end | |
| def generate_request_key(request) | |
| "#{request.request_method}_#{request.fullpath}" | |
| end |
| @socket_path = '/tmp/hawksi.sock' | ||
| start_unix_socket_server |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make the Unix socket path configurable
Currently, the socket path @socket_path = '/tmp/hawksi.sock' is hardcoded, which may lead to conflicts or reduce flexibility in different environments. Consider making it configurable through an environment variable or an initialization parameter.
Apply this diff to make the socket path configurable:
def initialize(app, logger: Logger.new('hawksi.log'), storage: FileStorage)
@app = app
@logger = logger
@storage = storage
@templates = {}
@templates_mutex = Mutex.new
- @socket_path = '/tmp/hawksi.sock'
+ @socket_path = ENV['HAWKSI_SOCKET_PATH'] || '/tmp/hawksi.sock'
start_unix_socket_server
end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @socket_path = '/tmp/hawksi.sock' | |
| start_unix_socket_server | |
| @socket_path = ENV['HAWKSI_SOCKET_PATH'] || '/tmp/hawksi.sock' | |
| start_unix_socket_server |
| status, headers, _temp = @app.call(env) | ||
|
|
||
| # Serve the template | ||
| headers['Etag'] = Digest::MD5.hexdigest(template) | ||
| return [status, headers, [template]] |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid unnecessary application calls when serving templates
When a template is found, the code still calls @app.call(env) to obtain the status and headers, which can be inefficient if the application is resource-intensive. Consider defining default status and headers to avoid unnecessary application calls.
Apply this diff to bypass calling the application when serving a template:
- status, headers, _temp = @app.call(env)
-
+ status = 200
+ headers = { 'Content-Type' => 'application/json' }
# Serve the template
headers['Etag'] = Digest::MD5.hexdigest(template)
return [status, headers, [template]]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| status, headers, _temp = @app.call(env) | |
| # Serve the template | |
| headers['Etag'] = Digest::MD5.hexdigest(template) | |
| return [status, headers, [template]] | |
| status = 200 | |
| headers = { 'Content-Type' => 'application/json' } | |
| # Serve the template | |
| headers['Etag'] = Digest::MD5.hexdigest(template) | |
| return [status, headers, [template]] |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
lib/command_executor.rb (1)
64-64: Improved feedback, but consider enhancing clarity and flexibility.The addition of this line improves user experience by providing immediate feedback in the console. Good use of
fetchwith a default value to prevent errors.However, consider the following suggestions:
- Add a prefix to the output to distinguish it from other console messages, e.g.,
puts "Response: #{result.fetch('response', '')}".- Consider making this console output configurable, allowing users to enable/disable it based on their needs, especially for scripted usage.
Here's a potential refactor to address these suggestions:
def log_command_result(result) if result['result'] == 'error' logger.error "Error during command execution: #{result['message']}" else logger.info "Command executed successfully. #{result}" puts "Response: #{result.fetch('response', '')}" if Hawksi.configuration.show_console_output end endThis assumes you add a
show_console_outputoption to your configuration. Don't forget to update the configuration class if you implement this suggestion.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation