Skip to content

Conversation

@akshayutture-augment
Copy link

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.

@akshayutture-augment
Copy link
Author

@greptileai

@greptile-apps
Copy link

greptile-apps bot commented Nov 14, 2025

Greptile Overview

Confidence Score: 3/5

  • This PR has a critical string mutation bug that needs to be fixed before merging
  • Score reflects one critical bug (mutating string with << operator on line 149) that could cause unexpected behavior, though the overall refactoring approach is sound and well-tested
  • Pay close attention to app/serializers/user_serializer.rb line 149 - the string concatenation bug must be fixed

Important Files Changed

File Analysis

Filename Score Overview
app/serializers/user_serializer.rb 3/5 Added website_name method with URL comparison logic. Contains string mutation bug with << operator on line 149
spec/serializers/user_serializer_spec.rb 5/5 Added comprehensive test coverage for new website_name method with three scenarios
app/assets/javascripts/discourse/controllers/user.js.es6 5/5 Removed client-side websiteName computed property, moving logic to backend serializer

Sequence Diagram

sequenceDiagram
    participant Browser
    participant Template as user.hbs
    participant Controller as UserController
    participant Serializer as UserSerializer
    participant Discourse as Discourse.current_hostname
    
    Browser->>Template: Request user profile page
    Template->>Controller: Access model.website_name
    Note over Controller: Removed websiteName<br/>computed property
    Controller->>Serializer: Fetch user data via API
    Serializer->>Serializer: Call website() method
    Serializer->>Serializer: Call website_name() method
    Serializer->>Discourse: Get current_hostname()
    Discourse-->>Serializer: Return instance hostname
    Serializer->>Serializer: Parse website URL with URI()
    Serializer->>Serializer: Compare website_host vs discourse_host
    alt Exact match (example.com == example.com)
        Serializer->>Serializer: Return host + path
    else Subdomain match (www.example.com vs forum.example.com)
        Serializer->>Serializer: Compare base domains
        alt Base domains match
            Serializer->>Serializer: Return host + path
        else Base domains differ
            Serializer->>Serializer: Return host only
        end
    else Parent domain check
        Serializer->>Serializer: Check if discourse ends with website
        alt Discourse is subdomain of website
            Serializer->>Serializer: Return host + path
        else Different domains
            Serializer->>Serializer: Return host only
        end
    end
    Serializer-->>Controller: Return serialized user with website_name
    Controller-->>Template: Provide model data
    Template-->>Browser: Render website link with website_name
Loading

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

5 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

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
Copy link

Choose a reason for hiding this comment

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

logic: String concatenation mutates discourse_host with << operator, which could cause side effects if discourse_host is reused

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
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/serializers/user_serializer.rb
Line: 149:149

Comment:
**logic:** String concatenation mutates `discourse_host` with `<<` operator, which could cause side effects if `discourse_host` is reused

```suggestion
      discourse_host.ends_with?("." + website_host) ? website_host + URI(website.to_s).path : website_host
```

How can I resolve this? If you propose a fix, please make it concise.

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