UX: show complete URL path if website domain is same as instance domain by everettbu · Pull Request #6 · ai-code-review-evaluation/discourse-copilot

everettbu · 2025-07-26T19:13:54Z

Test 6

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copilot

Pull Request Overview

This PR implements a UX improvement to show complete URL paths when a user's website domain matches the Discourse instance domain, instead of just showing the hostname. This allows users to see more specific website information when the domains are related.

Key changes:

Moved website name logic from frontend to backend serializer for better performance and consistency
Enhanced domain comparison logic to handle parent/subdomain relationships
Updated frontend template to use the new serialized website_name field

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
`app/serializers/user_serializer.rb`	Added `website_name` method with domain comparison logic and path inclusion
`spec/serializers/user_serializer_spec.rb`	Added comprehensive test cases for different domain scenarios
`app/assets/javascripts/discourse/templates/user/user.hbs`	Updated template to use model.website_name instead of controller property
`app/assets/javascripts/discourse/controllers/user.js.es6`	Removed obsolete websiteName computed property
`app/assets/javascripts/discourse/models/user.js.es6`	Fixed incorrect JSDoc comment

Copilot · 2025-07-30T19:01:23Z

  end

+  def website_name
+    website_host = URI(website.to_s).host rescue nil


Using rescue nil in inline form can hide different types of exceptions. Consider using explicit exception handling with begin/rescue to catch specific URI parsing errors.

Suggested change

website_host = URI(website.to_s).host rescue nil

website_host = begin

URI(website.to_s).host

rescue URI::InvalidURIError

nil

end

Copilot · 2025-07-30T19:01:24Z

+    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


The condition is complex and the domain splitting logic is repeated multiple times. Consider extracting the domain level calculation into a helper method to improve readability and reduce duplication.

Suggested change

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

compare_domains(website_host, discourse_host, URI(website.to_s).path)

end

private

def compare_domains(website_host, discourse_host, website_path)

if website_host == discourse_host

# example.com == example.com

website_host + website_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 + website_path : website_host

else

# example.com == forum.example.com

discourse_host.ends_with?("." << website_host) ? website_host + website_path : website_host

Copilot · 2025-07-30T19:01:24Z

+    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


URI parsing is called multiple times for the same website URL. Consider parsing the URI once and storing the result to avoid redundant parsing operations.

Suggested change

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

parsed_website = URI(website.to_s) rescue nil

website_host = parsed_website&.host

discourse_host = Discourse.current_hostname

return if website_host.nil?

if website_host == discourse_host

# example.com == example.com

website_host + parsed_website.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 + parsed_website.path : website_host

else

# example.com == forum.example.com

discourse_host.ends_with?("." << website_host) ? website_host + parsed_website.path : website_host

Copilot · 2025-07-30T19:01:24Z

+      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


The string concatenation using << modifies the original string. Consider using + for string concatenation to avoid side effects: discourse_host.ends_with?("." + website_host).

Suggested change

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

UX: show complete URL path if website domain is same as instance domain

267d8be

Copilot AI review requested due to automatic review settings July 26, 2025 19:13

Copilot AI reviewed Jul 26, 2025

everettbu requested a review from Copilot July 28, 2025 20:03

Copilot AI reviewed Jul 28, 2025

everettbu requested a review from Copilot July 28, 2025 20:06

This comment was marked as outdated.

Sign in to view

everettbu requested a review from Copilot July 30, 2025 19:00

Copilot AI reviewed Jul 30, 2025

View reviewed changes

akshayutture-augment mentioned this pull request Nov 14, 2025

UX: show complete URL path if website domain is same as instance domain ai-code-review-evaluations/copilot-discourse#6

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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#6
everettbu wants to merge 1 commit into
url-handling-pre-copilotfrom
url-handling-post-copilot

everettbu commented Jul 26, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

This comment was marked as outdated.

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Jul 30, 2025

Uh oh!

Copilot AI Jul 30, 2025

Uh oh!

Copilot AI Jul 30, 2025

Uh oh!

Copilot AI Jul 30, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	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

Conversation

everettbu commented Jul 26, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Jul 30, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 30, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 30, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 30, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants