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

[ShashankFC]

Test 6nn---nReplicated from ai-code-review-evaluation/discourse-greptile#6

[ShashankFC]

@claude

[claude]

Claude finished @ShashankFC's task in 1m 48s —— View job

---

Code Review Complete

I've reviewed this PR which implements UX improvements to show the complete URL path when a user's website domain matches the instance domain.

---

Summary

The PR refactors website name display logic by:

• Removing client-side logic from the JavaScript controller (user.js.es6:17-23)
• Moving the logic to server-side serialization (user_serializer.rb:137-151)
• Adding comprehensive test coverage (user_serializer_spec.rb:76-91)

This follows good architectural practice by handling presentation logic on the server.

---

Issues & Concerns

1. Logic Complexity & Edge Cases (user_serializer.rb:137-151)

The website_name method has complex nested conditionals that could be simplified. The current logic has potential issues:

Line 144-146: Complex multi-domain comparison logic

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

Concerns:

• Readability: The ternary operator on line 146 makes this hard to read
• Performance: Multiple .split('.') calls on the same strings (discourse_host is split 3 times!)
• Edge case: What happens with URLs like sub1.sub2.example.com?

Line 149: Potential bug with string concatenation

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

Using << modifies the string in place, which could cause unexpected behavior. Should use "." + website_host instead.

Recommendation: Refactor for clarity and cache split results:

def website_name
  website_host = URI(website.to_s).host rescue nil
  discourse_host = Discourse.current_hostname
  return if website_host.nil?
  
  website_parts = website_host.split('.')
  discourse_parts = discourse_host.split('.')
  path = URI(website.to_s).path
  
  # Exact match
  return website_host + path if website_host == discourse_host
  
  # Same level subdomains (e.g., www.example.com vs forum.example.com)
  if website_parts.length == discourse_parts.length && discourse_parts.length > 2
    return website_host + path if website_parts[1..-1] == discourse_parts[1..-1]
  end
  
  # Website domain is parent of discourse domain
  return website_host + path if discourse_host.ends_with?("." + website_host)
  
  website_host
end

Fix readability & performance →%20Cache%20split%20results%202)%20Replace%20ternary%20with%20early%20returns%203)%20Fix%20string%20concatenation%20bug%20on%20line%20149%20using%20%2B%20instead%20of%20%3C%3C&repo=AI-Code-Review-Evals/claude_code-discourse)

2. Missing Error Handling (user_serializer.rb:138)

Line 138: Silent exception rescue could hide errors

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

While this prevents crashes from malformed URLs, it silently swallows all exceptions (including ArgumentError, URI::InvalidURIError, etc.). This makes debugging difficult.

Recommendation: Be explicit about what you're rescuing:

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

3. Documentation Missing (user_serializer.rb:137-151)

The method lacks documentation explaining:

• The purpose and return value
• The different cases it handles
• Examples of inputs/outputs

Recommendation: Add method documentation:

# Returns a display name for the user's website
# - Returns hostname only if website is on a different domain
# - Returns hostname + path if website is on the same domain or subdomain
# 
# Examples:
#   website: "http://example.com/user", discourse: "discourse.org" => "example.com"
#   website: "http://example.com/user", discourse: "example.com" => "example.com/user"
#   website: "http://example.com/user", discourse: "forums.example.com" => "example.com/user"
def website_name

4. Test Coverage Gap (user_serializer_spec.rb:82-90)

The tests cover 3 scenarios but miss important edge cases:

• Malformed URLs (empty path, trailing slashes, query parameters)
• Multi-level subdomains (sub1.sub2.example.com)
• Internationalized domain names (IDNs)
• URLs with ports (example.com:8080)
• What happens when path is just / vs /user?

Recommendation: Add tests for edge cases:

it "handles URL with trailing slash" do
  user.user_profile.website = 'http://example.com/user/'
  Discourse.stubs(:current_hostname).returns('example.com')
  expect(json[:website_name]).to eq 'example.com/user/'
end

it "handles URL with query parameters" do
  user.user_profile.website = 'http://example.com/user?ref=profile'
  Discourse.stubs(:current_hostname).returns('discourse.org')
  expect(json[:website_name]).to eq 'example.com'
end

---

Positive Aspects

✅ Good architecture: Moving display logic from client to server
✅ Comprehensive tests: Three main scenarios are well covered
✅ Backward compatibility: Changes don't break existing functionality
✅ Clean refactor: Properly removed unused controller method
✅ Consistent naming: website_name is clear and follows conventions

---

Minor Issues

5. Comment Typo (user.js.es6:62)

Line 62 comment should say @property profileBackground but this was already fixed in the commit, so this is just noting the fix was correct.

6. Template Consistency (templates/user/user.hbs:66-72)

The template change from websiteName to model.website_name is correct and consistent with Ember conventions.

---

Recommendations Summary

Priority:

1. 🔴 High: Fix the string concatenation bug (line 149) - could cause unexpected behavior
2. 🟡 Medium: Refactor for readability and performance - cache split results
3. 🟡 Medium: Improve error handling - be explicit about rescued exceptions
4. 🟢 Low: Add method documentation
5. 🟢 Low: Add edge case tests

---

Overall Assessment

This PR implements a valuable UX improvement with solid architectural decisions. The main concerns are around code complexity and potential edge cases in the domain matching logic. The refactor to move logic server-side is the right approach.

Verdict: Approve with suggested improvements for the string concatenation bug and readability.

---

Tasks

• Read repository guidelines (CLAUDE.md)
• Review changed files
• Analyze implementation
• Provide review feedback

---