UX: show complete URL path if website domain is same as instance domain #5
Conversation
|
@coderabbitai review |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThis pull request refactors website name display logic from the frontend JavaScript controller to the backend Ruby serializer. The Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
spec/serializers/user_serializer_spec.rb (1)
76-91: Good test coverage for the main scenarios.The tests comprehensively validate the domain comparison logic for the three primary cases: unrelated domains, matching domains, and subdomain relationships.
Consider adding tests for edge cases:
- Website path is "/" or empty (should probably omit the path)
- URLs with port numbers (e.g.,
http://example.com:3000/user)- Malformed URLs or missing protocols
it "returns just host when path is root" do user.user_profile.website = 'http://example.com/' Discourse.stubs(:current_hostname).returns('example.com') expect(json[:website_name]).to eq 'example.com' end it "handles URLs with port numbers" do user.user_profile.website = 'http://example.com:3000/user' Discourse.stubs(:current_hostname).returns('example.com') expect(json[:website_name]).to eq 'example.com/user' endapp/serializers/user_serializer.rb (2)
141-149: Consider normalizing paths to avoid trailing slashes.The current implementation appends the path directly without checking if it's just "/" or empty. This could result in display values like "example.com/" which may not be desired.
Consider adding path normalization:
if website_host == discourse_host # example.com == example.com - website_host + URI(website.to_s).path + path = URI(website.to_s).path + path.present? && path != '/' ? website_host + path : website_hostApply similar logic to lines 146 and 149 where paths are appended.
149-149: Use string interpolation instead of string mutation.Line 149 uses the
<<operator which mutates the string. String interpolation is more idiomatic and doesn't modify the original string.- discourse_host.ends_with?("." << website_host) ? website_host + URI(website.to_s).path : website_host + discourse_host.ends_with?(".#{website_host}") ? website_host + URI(website.to_s).path : website_host
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/assets/javascripts/discourse/controllers/user.js.es6(0 hunks)app/assets/javascripts/discourse/models/user.js.es6(1 hunks)app/assets/javascripts/discourse/templates/user/user.hbs(1 hunks)app/serializers/user_serializer.rb(2 hunks)spec/serializers/user_serializer_spec.rb(1 hunks)
💤 Files with no reviewable changes (1)
- app/assets/javascripts/discourse/controllers/user.js.es6
🧰 Additional context used
🪛 RuboCop (1.81.7)
app/serializers/user_serializer.rb
[convention] 137-151: Assignment Branch Condition size for website_name is too high. [<2, 35, 12> 37.05/23]
(Metrics/AbcSize)
[convention] 137-151: Cyclomatic complexity for website_name is too high. [8/7]
(Metrics/CyclomaticComplexity)
[convention] 137-151: Perceived complexity for website_name is too high. [9/8]
(Metrics/PerceivedComplexity)
[convention] 138-138: Avoid using rescue in its modifier form.
(Style/RescueModifier)
🔇 Additional comments (2)
app/assets/javascripts/discourse/models/user.js.es6 (1)
59-59: LGTM! Documentation fix aligns with the actual property name.The JSDoc comment now correctly documents the
profileBackgroundproperty instead of incorrectly referencingwebsiteName.app/assets/javascripts/discourse/templates/user/user.hbs (1)
66-73: LGTM! Template correctly migrated to use backend-provided attribute.The template now uses
model.website_namewhich is provided by the UserSerializer, aligning with the architectural change to move this logic from the frontend controller to the backend.
| def website_name | ||
| website_host = URI(website.to_s).host rescue nil | ||
| discourse_host = Discourse.current_hostname | ||
| return if website_host.nil? | ||
| if website_host == discourse_host | ||
| # example.com == example.com | ||
| website_host + URI(website.to_s).path | ||
| elsif (website_host.split('.').length == discourse_host.split('.').length) && discourse_host.split('.').length > 2 | ||
| # www.example.com == forum.example.com | ||
| website_host.split('.')[1..-1].join('.') == discourse_host.split('.')[1..-1].join('.') ? website_host + URI(website.to_s).path : website_host | ||
| else | ||
| # example.com == forum.example.com | ||
| discourse_host.ends_with?("." << website_host) ? website_host + URI(website.to_s).path : website_host | ||
| end | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Consider refactoring to reduce complexity.
The method has high cyclomatic complexity (8/7) and perceived complexity (9/8), making it harder to understand and maintain. Consider extracting helper methods or simplifying the conditional logic.
Here's a refactored version that's more readable:
def website_name
return unless website.present?
begin
uri = URI(website.to_s)
website_host = uri.host
return unless website_host
path = format_path(uri.path)
should_include_path?(website_host) ? "#{website_host}#{path}" : website_host
rescue URI::InvalidURIError
nil
end
end
private
def should_include_path?(website_host)
discourse_host = Discourse.current_hostname
return true if website_host == discourse_host
return true if domains_share_parent?(website_host, discourse_host)
false
end
def domains_share_parent?(website_host, discourse_host)
# Check if discourse is a subdomain of website
return true if discourse_host.ends_with?(".#{website_host}")
# Check if both are subdomains of the same parent (e.g., www.example.com vs forum.example.com)
website_parts = website_host.split('.')
discourse_parts = discourse_host.split('.')
if website_parts.length == discourse_parts.length && website_parts.length > 2
website_parts[1..-1] == discourse_parts[1..-1]
else
false
end
end
def format_path(path)
path.present? && path != '/' ? path : ''
endAs per coding guidelines (based on RuboCop Metrics violations).
🧰 Tools
🪛 RuboCop (1.81.7)
[convention] 137-151: Assignment Branch Condition size for website_name is too high. [<2, 35, 12> 37.05/23]
(Metrics/AbcSize)
[convention] 137-151: Cyclomatic complexity for website_name is too high. [8/7]
(Metrics/CyclomaticComplexity)
[convention] 137-151: Perceived complexity for website_name is too high. [9/8]
(Metrics/PerceivedComplexity)
[convention] 138-138: Avoid using rescue in its modifier form.
(Style/RescueModifier)
🤖 Prompt for AI Agents
In app/serializers/user_serializer.rb around lines 137 to 151 the website_name
method is overly complex; refactor by extracting small helper methods and
handling URI parsing errors: validate website presence first, parse URI inside a
rescue for URI::InvalidURIError, compute website_host and path (returning empty
string for nil or "/" paths), and replace the large conditional with a
should_include_path?(website_host) helper that returns true when hosts are
identical, when discourse is a subdomain of the website, or when both hosts are
subdomains of the same parent (extracted into domains_share_parent?); finally
return "#{website_host}#{path}" when should_include_path? is true otherwise
website_host, keeping behavior identical but reducing cyclomatic complexity.
| end | ||
|
|
||
| def website_name | ||
| website_host = URI(website.to_s).host rescue nil |
There was a problem hiding this comment.
Replace rescue modifier with proper error handling.
Using rescue in modifier form is not idiomatic Ruby and can hide unexpected errors. It's better to use a proper rescue block or validate the input before parsing.
Apply this diff to use proper error handling:
- website_host = URI(website.to_s).host rescue nil
- discourse_host = Discourse.current_hostname
- return if website_host.nil?
+ discourse_host = Discourse.current_hostname
+
+ begin
+ website_host = URI(website.to_s).host
+ return if website_host.nil?
+ rescue URI::InvalidURIError
+ return nil
+ endAs per coding guidelines (based on RuboCop Style/RescueModifier).
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 RuboCop (1.81.7)
[convention] 138-138: Avoid using rescue in its modifier form.
(Style/RescueModifier)
🤖 Prompt for AI Agents
In app/serializers/user_serializer.rb around line 138, replace the
modifier-style rescue on URI(website.to_s).host with explicit error handling:
wrap the URI parsing in a begin...rescue block (or use URI.parse) and rescue
only the expected exceptions such as URI::InvalidURIError (and optionally
ArgumentError), returning nil if parsing fails; alternatively validate website
presence/format before calling URI to avoid raising — do not use the rescue
modifier.
Test 6
Summary by CodeRabbit
Bug Fixes
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.
Replicated from ai-code-review-evaluation/discourse-coderabbit#6