Skip to content

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

Open
ShashankFC wants to merge 1 commit into
url-handling-prefrom
url-handling-post
Open

UX: show complete URL path if website domain is same as instance domain#5
ShashankFC wants to merge 1 commit into
url-handling-prefrom
url-handling-post

Conversation

@ShashankFC

Copy link
Copy Markdown

Test 6

Summary by CodeRabbit

  • Refactor

    • Reorganized code architecture to improve maintainability and better separate concerns.
  • Tests

    • Enhanced test coverage for website domain name handling with new test cases covering different domain matching scenarios, including host name derivation and path normalization based on domain configuration relationships.

✏️ Tip: You can customize this high-level summary in your review settings.


Replicated from ai-code-review-evaluation/discourse-coderabbit#6

@ShashankFC ShashankFC requested a review from Copilot January 30, 2026 10:24

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refactors the handling of website domain names in user profiles to improve URL path display logic. When a user's website domain matches or is a parent of the instance domain, the complete URL path is now shown instead of just the hostname.

Changes:

  • Moved website name logic from frontend controller to backend serializer
  • Enhanced domain matching to handle same domain, sibling domains, and parent-child domain relationships
  • Fixed incorrect JSDoc comment for profileBackground property

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 included it in serialized attributes
spec/serializers/user_serializer_spec.rb Added comprehensive test cases for different domain matching scenarios
app/assets/javascripts/discourse/templates/user/user.hbs Updated template to reference model.website_name from serializer instead of controller property
app/assets/javascripts/discourse/controllers/user.js.es6 Removed websiteName computed property as logic moved to backend
app/assets/javascripts/discourse/models/user.js.es6 Corrected JSDoc comment from websiteName to profileBackground
Comments suppressed due to low confidence (1)

app/serializers/user_serializer.rb:1

  • The URI parsing operation URI(website.to_s).path is repeated multiple times. Extract it to a local variable at the beginning of the method to improve readability and avoid redundant parsing.
class UserSerializer < BasicUserSerializer

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

end

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

Copilot AI Jan 30, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using inline rescue is discouraged as it can mask unexpected errors. Consider using a begin-rescue-end block or extracting URI parsing to a separate method with explicit error handling.

Copilot uses AI. Check for mistakes.
Comment on lines +144 to +146
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

Copilot AI Jan 30, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The domain splitting operations website_host.split('.') and discourse_host.split('.') are called multiple times. Extract these to local variables to improve performance and readability.

Copilot uses AI. Check for mistakes.
Comment on lines +144 to +149
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

Copilot AI Jan 30, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic for domain matching is complex and would benefit from more detailed comments explaining the conditions being checked. The current comments show examples but don't explain why these specific checks are needed or what edge cases they handle.

Copilot uses AI. Check for mistakes.
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

Copilot AI Jan 30, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The string concatenation using << modifies the original string object. Use + instead: discourse_host.ends_with?('.' + website_host) to avoid potential side effects.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants