-
Notifications
You must be signed in to change notification settings - Fork 9
UX: show complete URL path if website domain is same as instance domain #6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: url-handling-pre
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Greptile Summary
This PR implements a UX improvement to make website URLs in user profiles more contextual and informative. The core change moves website name computation from client-side to server-side logic that intelligently displays either just the hostname (for external domains) or the complete URL path (when the website domain matches or is related to the Discourse instance domain).
The implementation spans several layers:
- Backend Logic: A new
website_nameattribute is added toUserSerializerwith sophisticated domain comparison logic that handles exact matches, same-level subdomains, and parent-child domain relationships - Frontend Cleanup: The client-side
websiteNamecomputed property is removed from the user controller, eliminating redundant processing - Template Updates: The user profile template now uses
model.website_namedirectly from the serializer instead of the controller's computed property - Documentation Fix: A minor JSDoc correction fixes an incorrect property name reference
- Test Coverage: Comprehensive tests validate the domain matching scenarios
This change is particularly valuable for corporate or organizational Discourse instances where users often link to other pages on the same domain. Instead of showing redundant hostnames like "example.com" when the forum itself is on example.com, users now see the meaningful path information like "example.com/user".
PR Description Notes:
- The PR description "Test 6" is not descriptive and should explain the UX improvement being implemented
Confidence score: 2/5
- This PR has significant implementation issues that could cause runtime errors and incorrect behavior
- The domain comparison logic in
UserSerializer#website_namehas several problematic areas including unsafe string operations, complex subdomain logic that may fail edge cases, and potential for incorrect domain matching - Files needing attention:
app/serializers/user_serializer.rb(critical domain logic issues),spec/serializers/user_serializer_spec.rb(test coverage gaps)
5 files reviewed, 2 comments
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: String concatenation with << modifies the original string. Use + or string interpolation to avoid mutating Discourse.current_hostname
| 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 |
| end | ||
|
|
||
| def website_name | ||
| website_host = URI(website.to_s).host rescue nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: URI parsing is called twice for the same website URL. Consider storing URI(website.to_s) in a variable for efficiency
Test 6