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

[akshayutture-augment]

Test 6

---

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

[akshayutture-augment]

@claude review this PR

[akshayutture-augment]

@claude review this PR

[claude]

Claude finished @akshayutture-augment's task —— View job

---

PR Review Complete ✓

Todo List:

• Read repository guidelines (CLAUDE.md)
• Analyze PR diff and changes
• Review modified files for issues
• Provide comprehensive feedback

---

Summary

This PR refactors the website display logic from client-side (JavaScript) to server-side (Ruby), showing the complete URL path when the website domain matches the Discourse instance domain. The implementation is generally solid with good test coverage, but there are several issues that need attention.

---

Critical Issues

1. String Mutation Bug in Ruby Code 🔴

Location: app/serializers/user_serializer.rb:149

discourse_host.ends_with?("." << website_host)

Problem: The << operator mutates website_host by prepending a . to it. This is a side effect that could cause unexpected behavior and makes the code harder to reason about.

Fix: Use string concatenation instead:

discourse_host.ends_with?("." + website_host)

2. Incomplete Domain Matching Logic 🟡

Location: app/serializers/user_serializer.rb:137-150

The current logic has gaps:

Case 1: website_host == discourse_host ✅ (works correctly)

• Example: example.com == example.com → shows full path

Case 2: Multi-level subdomain comparison ⚠️ (partially works)

• Line 144: Only checks when both have same number of domain parts AND more than 2 parts
• Example: www.example.com vs forum.example.com ✅ works
• Example: subdomain.example.com vs example.com ❌ doesn't match (falls through to Case 3)

Case 3: Parent domain check ⚠️ (has issues)

• Line 149: Checks if discourse_host ends with website_host
• Example: forums.example.com vs example.com ✅ works
• Example: example.com vs subdomain.example.com ❌ would incorrectly match as parent

Missing test case: What happens when website = "http://subdomain.example.com/path" and discourse_host = "example.com"? The current logic would show just the hostname, but the test at line 87-90 suggests it should show the full path.

3. Inconsistent Documentation 🟡

Location: app/assets/javascripts/discourse/models/user.js.es6:56-61

The JSDoc comment for profileBackground still references the old @property websiteName instead of @property profileBackground. While this was technically incorrect before this PR, it shows the property name was being used elsewhere. This should be fixed.

---

Medium Priority Issues

4. Missing Error Handling 🟡

Location: app/serializers/user_serializer.rb:138

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

Using inline rescue nil is generally discouraged as it silently swallows all exceptions, including unexpected ones. Consider:

begin
  website_host = URI(website.to_s).host
rescue URI::InvalidURIError
  return nil
end

This makes the error handling explicit and only catches the expected exception type.

5. Potential Nil Safety Issue 🟡

Location: app/serializers/user_serializer.rb:143

website_host + URI(website.to_s).path

If URI(website.to_s).path raises an exception (though unlikely after line 138), this could fail. Consider extracting the URI parsing once at the beginning:

def website_name
  return nil unless website.present?
  
  begin
    uri = URI(website.to_s)
    website_host = uri.host
    return nil if website_host.nil?
    
    discourse_host = Discourse.current_hostname
    # ... rest of logic
  rescue URI::InvalidURIError
    nil
  end
end

6. Test Coverage Gap 🟡

Location: spec/serializers/user_serializer_spec.rb:76-91

Missing test cases:

• URL with query parameters: http://example.com/user?id=123
• URL with fragments: http://example.com/user#section
• Root path: http://example.com/
• Complex subdomain scenarios: a.b.example.com vs c.d.example.com
• Edge case: example.com (website) vs subdomain.example.com (discourse) - should show hostname only
• Case sensitivity: what if domains differ only by case?

---

Low Priority / Style Issues

7. Code Organization 🔵

The website_name method is quite complex with nested conditionals. Consider extracting helper methods:

def website_name
  return nil unless website.present?
  
  uri = parse_website_uri
  return nil unless uri
  
  website_host = uri.host
  discourse_host = Discourse.current_hostname
  
  should_show_full_path?(website_host, discourse_host) ? 
    website_host + uri.path : 
    website_host
end

private

def parse_website_uri
  URI(website.to_s)
rescue URI::InvalidURIError
  nil
end

def should_show_full_path?(website_host, discourse_host)
  return true if website_host == discourse_host
  return true if same_parent_domain?(website_host, discourse_host)
  return true if website_is_parent_of_discourse?(website_host, discourse_host)
  false
end

def same_parent_domain?(host1, host2)
  parts1 = host1.split('.')
  parts2 = host2.split('.')
  return false unless parts1.length == parts2.length && parts1.length > 2
  parts1[1..-1] == parts2[1..-1]
end

def website_is_parent_of_discourse?(website_host, discourse_host)
  discourse_host.ends_with?("." + website_host)
end

8. Comment Quality 🔵

Location: app/serializers/user_serializer.rb:142, 145, 148

The inline comments are helpful but could be more descriptive:

# Both domains are identical, show full path
# Sibling subdomains (www.example.com vs forum.example.com), show full path if same parent
# Website domain is parent of discourse domain (example.com parent of forums.example.com)

---

Positive Aspects ✨

1. Good separation of concerns: Moving logic from client to server is the right approach
2. Backward compatible: Removes unused client-side code cleanly
3. Well-tested: Comprehensive test suite covering main scenarios
4. Clear intent: The PR purpose is well-implemented despite the issues above
5. Template updates: Correctly migrated from computed property to model attribute

---

Recommendations

Must Fix Before Merge:

1. Fix the string mutation bug (Issue FEATURE: Can edit category/host relationships for embedding #1)
2. Review and fix the domain matching logic (Issue FEATURE: Localization fallbacks (server-side) #2)
3. Add proper error handling (Issue scale-color $lightness must use $secondary for dark themes #4)

Should Fix:
4. Add missing test cases (Issue #6)
5. Fix the JSDoc comment (Issue #3)

Consider:
6. Refactor for better readability (Issue #7)

---

Questions for Author

1. What should happen when the website is subdomain.example.com and Discourse is at example.com? Should it show the full path or just hostname?
2. Have you tested this with internationalized domain names (IDN)?
3. Should the path be shown when query parameters or fragments are present?

---