UX: show complete URL path if website domain is same as instance domain#6
UX: show complete URL path if website domain is same as instance domain#6akshayutture-augment wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the display of user website URLs in the UI by showing the complete URL path when the website domain matches the instance domain. Previously, only the domain name was displayed regardless of the path.
Key Changes:
- Moved
website_namecomputation from frontend controller to backend serializer with enhanced logic - Added conditional path display based on domain comparison between user website and instance
- Updated frontend templates to use serializer-provided
website_nameinstead of computed property
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| app/serializers/user_serializer.rb | Added website_name attribute with logic to include path when domains match |
| app/assets/javascripts/discourse/controllers/user.js.es6 | Removed client-side websiteName computed property |
| app/assets/javascripts/discourse/templates/user/user.hbs | Updated template to use model.website_name from serializer |
| app/assets/javascripts/discourse/models/user.js.es6 | Fixed incorrect JSDoc comment reference |
| spec/serializers/user_serializer_spec.rb | Added comprehensive test coverage for new website_name logic |
Comments suppressed due to low confidence (1)
app/serializers/user_serializer.rb:1
URI(website.to_s)is parsed multiple times within the same method. Parse the URI once at the beginning and reuse the result to avoid redundant parsing overhead.
class UserSerializer < BasicUserSerializer
| end | ||
|
|
||
| def website_name | ||
| website_host = URI(website.to_s).host rescue nil |
There was a problem hiding this comment.
Using inline rescue obscures potential errors and makes debugging difficult. Consider using a begin-rescue-end block or extracting this parsing logic into a separate method with explicit error handling.
| 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 |
There was a problem hiding this comment.
This conditional logic is complex and difficult to understand. The repeated .split('.') operations and nested ternary make the code hard to maintain. Consider extracting this subdomain comparison logic into a well-named helper method with clear documentation.
| 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 | |
| if hosts_equivalent_with_subdomain?(website_host, discourse_host) | |
| website_host + URI(website.to_s).path | |
| else | |
| website_host | |
| end | |
| end | |
| private | |
| # Determines if the website host and discourse host are equivalent, accounting for subdomains. | |
| # Returns true if: | |
| # - The hosts are exactly equal. | |
| # - Both hosts have the same number of subdomains (and more than 2), and their root domains match. | |
| # - The discourse host ends with the website host (e.g., forum.example.com ends with example.com). | |
| def hosts_equivalent_with_subdomain?(website_host, discourse_host) | |
| return false if website_host.nil? || discourse_host.nil? | |
| return true if website_host == discourse_host | |
| website_parts = website_host.split('.') | |
| discourse_parts = discourse_host.split('.') | |
| if website_parts.length == discourse_parts.length && discourse_parts.length > 2 | |
| # www.example.com == forum.example.com | |
| return website_parts[1..-1].join('.') == discourse_parts[1..-1].join('.') | |
| else | |
| # example.com == forum.example.com | |
| return discourse_host.end_with?("." + website_host) |
| 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 |
There was a problem hiding this comment.
Using << for string concatenation mutates the string object, which can cause unexpected behavior with frozen strings. Use + or string interpolation instead: discourse_host.ends_with?('.' + website_host)
| 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 |
Test 6
Replicated from ai-code-review-evaluation/discourse-copilot#6