create UI #1
create UI #1
Conversation
WalkthroughWalkthroughThe pull request introduces several changes across multiple files, primarily focusing on updating dependency management and enhancing configuration handling. The Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant FileUploader
participant Configuration
User->>FileUploader: Initiate file upload
FileUploader->>Configuration: Retrieve upload URL
Configuration-->>FileUploader: Return dynamic upload URL
FileUploader->>User: Complete file upload
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: 15
Outside diff range and nitpick comments (2)
lib/hawksi/configuration.rb (2)
3-3: Add documentation for configuration attributesAdding documentation comments for the configuration attributes would improve code readability and help other developers understand the purpose of each setting.
15-17: Simplify environment variable retrievalThe
get_envmethod can be streamlined for clarity. UsingENV[key]directly and handlingniland empty strings can make the code cleaner.Apply this diff to simplify the method:
def get_env(key) - env_value = ENV.fetch(key, '').strip - env_value.empty? ? nil : env_value + env_value = ENV[key]&.strip + env_value unless env_value.nil? || env_value.empty? endThis change removes the need to fetch with a default and handles cases where the environment variable is not set or is empty after stripping.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
Gemfile.lockis excluded by!**/*.lock
Files selected for processing (8)
- Gemfile (1 hunks)
- lib/command_executor.rb (1 hunks)
- lib/file_uploader.rb (2 hunks)
- lib/hawksi.rb (1 hunks)
- lib/hawksi/configuration.rb (1 hunks)
- lib/mocksi_handler.rb (1 hunks)
- lib/request_interceptor.rb (2 hunks)
- spec/lib/hawksi/configuration_spec.rb (1 hunks)
Additional context used
rubocop
Gemfile
[convention] 10-10: Gems should be sorted in an alphabetical order within their section of the Gemfile. Gem
cursesshould appear beforehttpx.(Bundler/OrderedGems)
lib/hawksi.rb
[warning] 6-12: Combine "" and " _ _ \n /\ /\__ ___ | | _____()\n / // / ` \ \ /\ / / |/ / __| |\n/ __ / (| |\ V V /| <\__ \ |\n\/ // \,| \/\/ ||\\/_|\n\n" into a single string literal, rather than using implicit string concatenation.
(Lint/ImplicitStringConcatenation)
[warning] 6-12: Combine " _ _ \n /\ /\__ ___ | | _____()\n / // / ` \ \ /\ / / |/ / __| |\n/ __ / (| |\ V V /| <\__ \ |\n\/ // \,| \/\/ ||\\/_|\n\n" and "" into a single string literal, rather than using implicit string concatenation.
(Lint/ImplicitStringConcatenation)
lib/mocksi_handler.rb
[convention] 11-82: Assignment Branch Condition size for handle is too high. [<21, 52, 21> 59.88/23]
(Metrics/AbcSize)
[convention] 11-82: Cyclomatic complexity for handle is too high. [19/7]
(Metrics/CyclomaticComplexity)
[convention] 11-82: Perceived complexity for handle is too high. [20/8]
(Metrics/PerceivedComplexity)
Additional comments not posted (2)
lib/command_executor.rb (1)
11-11: Ensure consistent usage of the configuration for the endpoint URLGreat to see that the
@endpoint_urlis now set usingHawksi.configuration.reactor_urlfor better flexibility. Please verify that there are no remaining hardcoded URLs or usages ofENV['MOCKSI_REACTOR_URL']elsewhere in the codebase to maintain consistency.Run the following script to check for any remaining references to
ENV['MOCKSI_REACTOR_URL'] or hardcoded URLs:Verification successful
Verification successful: Consistent usage of configuration for endpoint URL
The changes made to use
Hawksi.configuration.reactor_urlare consistent with the rest of the codebase. No remaining hardcoded URLs or inconsistent usages ofENV['MOCKSI_REACTOR_URL']were found in the production code. The configuration file properly handles default values and environment variable overrides.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find instances of ENV['MOCKSI_REACTOR_URL'] or hardcoded reactor URLs. # Test: Search for usage of the old environment variable. rg --type ruby "ENV\['MOCKSI_REACTOR_URL'\]" # Test: Search for hardcoded URLs matching the reactor URL pattern. rg --type ruby "https?://.*reactor\.mocksi\.io"Length of output: 288
Script:
#!/bin/bash # Description: Perform a more comprehensive search for endpoint URL usage # Search for variations of the environment variable name echo "Searching for variations of the environment variable name:" rg --type ruby "ENV\['.*REACTOR_URL'\]" # Search for other URL patterns that might be related to the reactor URL echo "Searching for other URL patterns:" rg --type ruby "https?://.*\.(mocksi|hawksi)\.(io|ai|com)" # Search for usage of Hawksi.configuration.reactor_url echo "Searching for usage of Hawksi.configuration.reactor_url:" rg --type ruby "Hawksi\.configuration\.reactor_url"Length of output: 3302
spec/lib/hawksi/configuration_spec.rb (1)
58-70: Isolate environment variable changes to prevent side effectsAs with the previous context, directly modifying environment variables in your tests can cause unintended side effects. Ensure that the environment variables are reset properly even if an exception occurs. Using
aroundhooks or environment management gems will help maintain test isolation and reliability.
Gemfile
Outdated
| @@ -7,6 +7,7 @@ gem 'puma' | |||
| gem 'thor' | |||
| gem 'json' | |||
| gem 'httpx', '~> 1.3' | |||
| gem 'curses' | |||
There was a problem hiding this comment.
Order gems alphabetically for consistency
According to RuboCop's Bundler/OrderedGems convention, gems should be sorted in alphabetical order within their section of the Gemfile. Moving gem 'curses' before gem 'httpx' would adhere to this standard.
Apply this diff to reorder the gems:
gem 'json'
gem 'httpx', '~> 1.3'
gem 'curses'Change to:
gem 'json'
gem 'curses'
gem 'httpx', '~> 1.3'Committable suggestion was skipped due to low confidence.
Tools
rubocop
[convention] 10-10: Gems should be sorted in an alphabetical order within their section of the Gemfile. Gem
cursesshould appear beforehttpx.(Bundler/OrderedGems)
lib/hawksi.rb
Outdated
| /\\ /\\__ ___ _| | _____(_) | ||
| / /_/ / _` \\ \\ /\\ / / |/ / __| | | ||
| / __ / (_| |\\ V V /| <\\__ \\ | | ||
| \\/ /_/ \\__,_| \\_/\\_/ |_|\\_\\___/_| | ||
|
|
||
| """ |
There was a problem hiding this comment.
Use correct syntax for multiline strings in Ruby
Ruby does not support triple quotes (""") for multiline strings. To define a multiline string, you can use a heredoc (<<~BANNER).
Apply this fix to correct the syntax:
-BANNER = """ _ _
- /\ /\__ ___ _| | _____(_)
- / /_/ / _` \ \ /\ / / |/ / __| |
-/ __ / (_| |\ V V /| <\__ \ |
-\/ /_/ \__,_| \_/\_/ |_|\_\___/_|
-
-"""
+BANNER = <<~BANNER
+ _ _
+ /\ /\__ ___ _| | _____(_)
+ / /_/ / _` \ \ /\ / / |/ / __| |
+/ __ / (_| |\ V V /| <\__ \ |
+\/ /_/ \__,_| \_/\_/ |_|\_\___/_|
+
+BANNERCommittable 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.
| BANNER = """ _ _ | |
| /\\ /\\__ ___ _| | _____(_) | |
| / /_/ / _` \\ \\ /\\ / / |/ / __| | | |
| / __ / (_| |\\ V V /| <\\__ \\ | | |
| \\/ /_/ \\__,_| \\_/\\_/ |_|\\_\\___/_| | |
| """ | |
| BANNER = <<~BANNER | |
| _ _ | |
| /\ /\__ ___ _| | _____(_) | |
| / /_/ / _` \ \ /\ / / |/ / __| | | |
| / __ / (_| |\ V V /| <\__ \ | | |
| \/ /_/ \__,_| \_/\_/ |_|\_\___/_| | |
| BANNER |
Tools
rubocop
[warning] 6-12: Combine "" and " _ _ \n /\ /\__ ___ | | _____()\n / // / ` \ \ /\ / / |/ / __| |\n/ __ / (| |\ V V /| <\__ \ |\n\/ // \,| \/\/ ||\\/_|\n\n" into a single string literal, rather than using implicit string concatenation.
(Lint/ImplicitStringConcatenation)
[warning] 6-12: Combine " _ _ \n /\ /\__ ___ | | _____()\n / // / ` \ \ /\ / / |/ / __| |\n/ __ / (| |\ V V /| <\__ \ |\n\/ // \,| \/\/ ||\\/_|\n\n" and "" into a single string literal, rather than using implicit string concatenation.
(Lint/ImplicitStringConcatenation)
| class << self | ||
| def configuration | ||
| @configuration ||= Configuration.new | ||
| end |
There was a problem hiding this comment.
Ensure thread-safe initialization of the configuration
If the application is running in a multithreaded environment, lazy initialization of @configuration could lead to race conditions. Eagerly initializing the configuration can prevent potential issues.
Apply this diff to initialize the configuration when the module is loaded:
class << self
+ @configuration = Configuration.new
def configuration
- @configuration ||= Configuration.new
+ @configuration
endInitializing @configuration at the class level ensures it's set up before any threads access it.
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.
| class << self | |
| def configuration | |
| @configuration ||= Configuration.new | |
| end | |
| class << self | |
| @configuration = Configuration.new | |
| def configuration | |
| @configuration | |
| end |
| def initialize | ||
| @mocksi_server = get_env('MOCKSI_SERVER') || "https://app.mocksi.ai" | ||
| @reactor_url = get_env('MOCKSI_REACTOR_URL') || "https://api.mocksi.ai/api/v1/reactor" | ||
| @upload_url = get_env('MOCKSI_UPLOAD_URL') || "https://api.mocksi.ai/api/v1/upload" | ||
| @process_url = get_env('MOCKSI_PROCESS_URL') || "https://api.mocksi.ai/api/v1/process" | ||
| end |
There was a problem hiding this comment.
Consider refactoring to reduce code duplication
The initialize method assigns values to several instance variables using a similar pattern. Refactoring this can make the code more concise and maintainable.
Apply this diff to simplify the assignments:
def initialize
- @mocksi_server = get_env('MOCKSI_SERVER') || "https://app.mocksi.ai"
- @reactor_url = get_env('MOCKSI_REACTOR_URL') || "https://api.mocksi.ai/api/v1/reactor"
- @upload_url = get_env('MOCKSI_UPLOAD_URL') || "https://api.mocksi.ai/api/v1/upload"
- @process_url = get_env('MOCKSI_PROCESS_URL') || "https://api.mocksi.ai/api/v1/process"
+ {
+ mocksi_server: "https://app.mocksi.ai",
+ reactor_url: "https://api.mocksi.ai/api/v1/reactor",
+ upload_url: "https://api.mocksi.ai/api/v1/upload",
+ process_url: "https://api.mocksi.ai/api/v1/process"
+ }.each do |attr, default|
+ instance_variable_set("@#{attr}", get_env(attr.to_s.upcase) || default)
+ end
endThis refactoring uses a hash to store default values and iterates over it to set instance variables dynamically.
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 initialize | |
| @mocksi_server = get_env('MOCKSI_SERVER') || "https://app.mocksi.ai" | |
| @reactor_url = get_env('MOCKSI_REACTOR_URL') || "https://api.mocksi.ai/api/v1/reactor" | |
| @upload_url = get_env('MOCKSI_UPLOAD_URL') || "https://api.mocksi.ai/api/v1/upload" | |
| @process_url = get_env('MOCKSI_PROCESS_URL') || "https://api.mocksi.ai/api/v1/process" | |
| end | |
| def initialize | |
| { | |
| mocksi_server: "https://app.mocksi.ai", | |
| reactor_url: "https://api.mocksi.ai/api/v1/reactor", | |
| upload_url: "https://api.mocksi.ai/api/v1/upload", | |
| process_url: "https://api.mocksi.ai/api/v1/process" | |
| }.each do |attr, default| | |
| instance_variable_set("@#{attr}", get_env(attr.to_s.upcase) || default) | |
| end | |
| end |
lib/file_uploader.rb
Outdated
| @@ -19,7 +16,7 @@ def upload_files(tar_gz_files) | |||
|
|
|||
| def process_files | |||
| HTTPX.wrap do |client| | |||
| response = client.post(PROCESS_URL, headers: { "x-client-id" => @client_uuid }) | |||
| response = client.post(Hawksi.configuration.process_url, headers: { "x-client-id" => @client_uuid }) | |||
There was a problem hiding this comment.
Handle missing or invalid process_url configuration
Currently, the code assumes that Hawksi.configuration.process_url is set and valid. If it is nil or invalid, client.post may raise an exception. Consider adding a check to ensure that process_url is present and handle cases where it's missing.
Add exception handling for network errors
Unlike the post_file method, the process_files method does not have a rescue block to handle exceptions that may be raised during the network request. Consider adding exception handling to catch and log any network-related errors.
Apply this diff to add exception handling:
def process_files
HTTPX.wrap do |client|
+ begin
response = client.post(Hawksi.configuration.process_url, headers: { "x-client-id" => @client_uuid })
if response.is_a?(HTTPX::Response)
@logger.info "Processing uploaded files. Status: #{response.status}"
else
@logger.error "Failed to process files. Error: #{response.error}"
end
+ rescue => e
+ @logger.error "Exception occurred while processing files: #{e.message}"
+ end
end
endCommittable suggestion was skipped due to low confidence.
| it 'memoizes the configuration instance' do | ||
| config1 = Hawksi.configuration | ||
| config2 = Hawksi.configuration | ||
| expect(config1).to eq(config2) |
There was a problem hiding this comment.
Use equal matcher to check object identity instead of eq
In the test for memoization on line 14, you are using expect(config1).to eq(config2), which checks for value equality. To ensure that both config1 and config2 refer to the exact same object instance (i.e., memoization), you should use the equal matcher:
- expect(config1).to eq(config2)
+ expect(config1).to equal(config2)This change will assert that the two variables point to the same object, verifying the memoization effectively.
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.
| expect(config1).to eq(config2) | |
| expect(config1).to equal(config2) |
| if request.path.end_with?('/favicon.ico') | ||
| return MocksiHandler.handle(request) | ||
| end | ||
|
|
||
| if request.path.start_with?('/mocksi') || request.path.start_with?('/_') || request.path.start_with?('/api') | ||
| return MocksiHandler.handle(request) | ||
| end |
There was a problem hiding this comment.
Simplify Conditional Logic for Request Path Handling
The two conditional statements checking the request path can be combined to improve readability and maintainability. Since both conditions result in the same action (return MocksiHandler.handle(request)), you can merge them into a single condition using start_with? and end_with? methods.
Apply this diff to combine the conditions:
-if request.path.end_with?('/favicon.ico')
- return MocksiHandler.handle(request)
-end
-
-if request.path.start_with?('/mocksi') || request.path.start_with?('/_') || request.path.start_with?('/api')
- return MocksiHandler.handle(request)
-end
+if request.path.end_with?('/favicon.ico') || request.path.start_with?('/mocksi', '/_', '/api')
+ return MocksiHandler.handle(request)
+endThis refactor leverages Ruby's ability to pass multiple arguments to start_with?, simplifying the condition.
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.
| if request.path.end_with?('/favicon.ico') | |
| return MocksiHandler.handle(request) | |
| end | |
| if request.path.start_with?('/mocksi') || request.path.start_with?('/_') || request.path.start_with?('/api') | |
| return MocksiHandler.handle(request) | |
| end | |
| if request.path.end_with?('/favicon.ico') || request.path.start_with?('/mocksi', '/_', '/api') | |
| return MocksiHandler.handle(request) | |
| end |
| def handle(request) | ||
| if request.path == '/favicon.ico' | ||
| return [200, { 'Content-Type' => 'image/svg+xml' }, [HAWK_SVG]] | ||
| end | ||
|
|
||
| begin | ||
| # Get the mocksi server URL from configuration | ||
| mocksi_server_url = Hawksi.configuration.mocksi_server | ||
| raise "Mocksi server URL not configured" if mocksi_server_url.nil? || mocksi_server_url.empty? | ||
|
|
||
| # Prepare the full URL (mocksi_server + request path + query string) | ||
| target_uri = URI.join(mocksi_server_url, request.fullpath) | ||
|
|
||
| # Prepare headers from the request | ||
| headers = {} | ||
| request.env.each do |key, value| | ||
| if key.start_with?('HTTP_') | ||
| header_key = key.sub(/^HTTP_/, '').split('_').map(&:capitalize).join('-') | ||
| headers[header_key] = value | ||
| end | ||
| ## Yay for Rack's weirdness. See https://github.com/rack/rack/issues/1311 | ||
| if key == 'CONTENT_TYPE' | ||
| headers['Content-Type'] = value | ||
| end | ||
| if key == 'CONTENT_LENGTH' | ||
| headers['Content-Length'] = value | ||
| end | ||
| end | ||
|
|
||
| # Forward the cookies | ||
| if request.cookies.any? | ||
| headers['Cookie'] = request.cookies.map { |k, v| "#{k}=#{v}" }.join('; ') | ||
| end | ||
|
|
||
| # Initialize httpx with headers | ||
| http_client = HTTPX.with(headers: headers) | ||
|
|
||
| # Forward the body content if it's a POST or PUT request | ||
| body = nil | ||
| if %w[POST PUT].include?(request.request_method) | ||
| request.body.rewind | ||
| body = request.body.read | ||
| end | ||
|
|
||
| # Make the HTTP request using the appropriate method | ||
| response = http_client.request(request.request_method.downcase.to_sym, target_uri, body: body) | ||
|
|
||
| # Clone headers to allow modification | ||
| response_headers = response.headers.dup | ||
|
|
||
| # Check for chunked transfer encoding and remove it | ||
| if response_headers["transfer-encoding"]&.include?("chunked") | ||
| response_headers.delete("transfer-encoding") | ||
| end | ||
|
|
||
| # Handle gzip or deflate content-encoding if present | ||
| response_body = response.body.to_s | ||
| if response_headers["content-encoding"]&.include?("gzip") | ||
| response_body = safe_decompress_gzip(response_body) | ||
| response_headers.delete("content-encoding") # Remove content-encoding since the content is decompressed | ||
| elsif response_headers["content-encoding"]&.include?("deflate") | ||
| response_body = decompress_deflate(response_body) | ||
| response_headers.delete("content-encoding") # Remove content-encoding since the content is decompressed | ||
| end | ||
|
|
||
| # Return the response in a format compatible with Rack | ||
| [response.status, response_headers, [response_body]] | ||
| rescue => e | ||
| # Handle any errors that occur during the reverse proxy operation | ||
| [500, { 'Content-Type' => 'text/plain' }, ["Error: #{e.message}"]] | ||
| end | ||
| end |
There was a problem hiding this comment.
Consider refactoring handle method to reduce complexity
The handle method has high complexity metrics as indicated by static analysis tools:
- Assignment Branch Condition size: 59.88/23
- Cyclomatic Complexity: 19/7
- Perceived Complexity: 20/8
High complexity can make the code harder to read, maintain, and test. Consider refactoring by extracting some of the logic into smaller, reusable helper methods. This will improve readability and maintainability.
Tools
rubocop
[convention] 11-82: Assignment Branch Condition size for handle is too high. [<21, 52, 21> 59.88/23]
(Metrics/AbcSize)
[convention] 11-82: Cyclomatic complexity for handle is too high. [19/7]
(Metrics/CyclomaticComplexity)
[convention] 11-82: Perceived complexity for handle is too high. [20/8]
(Metrics/PerceivedComplexity)
| def decompress_deflate(body) | ||
| Zlib::Inflate.inflate(body) | ||
| end |
There was a problem hiding this comment.
Add exception handling to decompress_deflate method
If the body is not valid deflate-compressed data, Zlib::Inflate.inflate(body) may raise an exception, potentially crashing the application. Consider adding exception handling to handle invalid deflate data gracefully.
Apply this diff to fix the issue:
def decompress_deflate(body)
+ begin
Zlib::Inflate.inflate(body)
+ rescue Zlib::DataError
+ # If the body is not actually deflate-compressed, return the original body
+ body
+ end
endCommittable 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 decompress_deflate(body) | |
| Zlib::Inflate.inflate(body) | |
| end | |
| def decompress_deflate(body) | |
| begin | |
| Zlib::Inflate.inflate(body) | |
| rescue Zlib::DataError | |
| # If the body is not actually deflate-compressed, return the original body | |
| body | |
| end | |
| end |
| [response.status, response_headers, [response_body]] | ||
| rescue => e | ||
| # Handle any errors that occur during the reverse proxy operation | ||
| [500, { 'Content-Type' => 'text/plain' }, ["Error: #{e.message}"]] |
There was a problem hiding this comment.
Avoid exposing internal error messages to clients
Returning internal error messages to the client may expose sensitive information and pose a security risk. It's better to log the error internally and return a generic message to the client.
Apply this diff to fix the issue:
- [500, { 'Content-Type' => 'text/plain' }, ["Error: #{e.message}"]]
+ # Log the error message internally (implementation depends on your logging setup)
+ # logger.error(e.message)
+ [500, { 'Content-Type' => 'text/plain' }, ["An internal error occurred. Please try again later."]]Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (2)
lib/file_uploader.rb (2)
19-27: Improved error handling and response processingThe changes in the
process_filesmethod are a significant improvement. The addition of error handling and the refined response processing address the previous review comments and enhance the robustness of the code.Consider extracting the URL configuration check into a separate method for reusability:
def process_files + process_url = get_config_url('process_url') + return if process_url.nil? + HTTPX.wrap do |client| response = begin - client.post(Hawksi.configuration.process_url, headers: { "x-client-id" => @client_uuid }) + client.post(process_url, headers: { "x-client-id" => @client_uuid }) rescue => e @logger.error "Failed to process files. Error: #{e.message}" end # ... rest of the method end end +private + +def get_config_url(config_key) + url = Hawksi.configuration.send(config_key) + if url.nil? || url.empty? + @logger.error "Missing or invalid #{config_key} configuration" + return nil + end + url +endThis change would also address the past review comment about handling missing or invalid configuration.
Line range hint
6-6: Address the FIXME commentThere's a FIXME comment at the beginning of the
FileUploaderclass that suggests using a base URL for the upload and process URLs. Given the recent changes to useHawksi.configurationfor URLs, it would be appropriate to address this comment now.Consider implementing a base URL in the configuration and updating the
upload_urlandprocess_urlto use it. For example:# In Hawksi::Configuration def initialize @base_url = ENV['HAWKSI_BASE_URL'] || 'https://api.hawksi.com' @upload_url = "#{@base_url}/upload" @process_url = "#{@base_url}/process" end # Then in FileUploader, you can use Hawksi.configuration.upload_url and Hawksi.configuration.process_url as you're already doingThis change would address the FIXME comment and provide a more flexible configuration setup.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
Gemfile.lockis excluded by!**/*.lock
Files selected for processing (3)
- Gemfile (1 hunks)
- lib/file_uploader.rb (2 hunks)
- lib/hawksi.rb (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- Gemfile
- lib/hawksi.rb
| @@ -58,7 +60,7 @@ def upload_file(tar_gz_file) | |||
|
|
|||
| def post_file(client, tar_gz_file) | |||
| filename = File.basename(tar_gz_file) | |||
| client.post("#{UPLOAD_URL}?filename=#{filename}", | |||
| client.post("#{Hawksi.configuration.upload_url}?filename=#{filename}", | |||
There was a problem hiding this comment.
Dynamic configuration for upload URL
The change to use Hawksi.configuration.upload_url is a good improvement, aligning with the overall strategy of using dynamic configurations instead of hardcoded URLs.
However, there are two important issues from previous reviews that still need to be addressed:
- URI-encode the
filenameto handle special characters:
def post_file(client, tar_gz_file)
filename = File.basename(tar_gz_file)
+ encoded_filename = URI.encode_www_form_component(filename)
- client.post("#{Hawksi.configuration.upload_url}?filename=#{filename}",
+ client.post("#{Hawksi.configuration.upload_url}?filename=#{encoded_filename}",
headers: { "x-client-id" => @client_uuid },
body: File.read(tar_gz_file))
rescue => e
@logger.error "Failed to upload #{tar_gz_file}: #{e.message}"
nil
end- Handle missing or invalid
upload_urlconfiguration:
def post_file(client, tar_gz_file)
+ upload_url = get_config_url('upload_url')
+ return nil if upload_url.nil?
+
filename = File.basename(tar_gz_file)
encoded_filename = URI.encode_www_form_component(filename)
- client.post("#{Hawksi.configuration.upload_url}?filename=#{encoded_filename}",
+ client.post("#{upload_url}?filename=#{encoded_filename}",
headers: { "x-client-id" => @client_uuid },
body: File.read(tar_gz_file))
rescue => e
@logger.error "Failed to upload #{tar_gz_file}: #{e.message}"
nil
endThese changes will improve the robustness and reliability of the post_file method.
Committable suggestion was skipped due to low confidence.
Summary by CodeRabbit
New Features
cursesgem for enhanced terminal handling capabilities.Bug Fixes
Tests
Hawksi::Configurationclass to ensure proper functionality and configuration management.