Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,11 @@ export default Ember.Component.extend(StringBuffer, {
}.on('willDestroyElement'),

renderString(buffer) {
const title = this.get('title');
if (title) {
buffer.push("<h4 class='title'>" + title + "</h4>");
}

buffer.push("<h4 class='title'>" + this.get('title') + "</h4>");
buffer.push("<button class='btn standard dropdown-toggle' data-toggle='dropdown'>");
buffer.push(this.get('text'));
buffer.push("</button>");
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import ObjectController from "discourse/controllers/object";

export default ObjectController.extend({

stopNotificiationsText: function() {
return I18n.t("topic.unsubscribe.stop_notifications", { title: this.get("model.fancyTitle") });
}.property("model.fancyTitle"),
Comment on lines +5 to +7

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

# Find and display the controller file content
fd -t f "topic-unsubscribe.js.es6" --exec cat -n {}

Repository: AI-Code-Review-Evals/coderabbit-discourse

Length of output: 1261


🏁 Script executed:

# Find the template file and check its content
fd -t f "unsubscribe.hbs" --exec cat -n {}

Repository: AI-Code-Review-Evals/coderabbit-discourse

Length of output: 332


🏁 Script executed:

# Search for references to "stopNotif" in the codebase to see all usages
rg "stopNotif" -n

Repository: AI-Code-Review-Evals/coderabbit-discourse

Length of output: 295


Fix misspelled method name in controller and template: stopNotificiationsText should be stopNotificationsText.

The method name is missing a 'c' in "Notifications". Both the controller method and the template reference must be updated together to fix this consistently.

Proposed changes

Controller (app/assets/javascripts/discourse/controllers/topic-unsubscribe.js.es6):

-  stopNotificiationsText: function() {
+  stopNotificationsText: function() {
     return I18n.t("topic.unsubscribe.stop_notifications", { title: this.get("model.fancyTitle") });
   }.property("model.fancyTitle"),

Template (app/assets/javascripts/discourse/templates/topic/unsubscribe.hbs):

-    {{{stopNotificiationsText}}}
+    {{{stopNotificationsText}}}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
stopNotificiationsText: function() {
return I18n.t("topic.unsubscribe.stop_notifications", { title: this.get("model.fancyTitle") });
}.property("model.fancyTitle"),
stopNotificationsText: function() {
return I18n.t("topic.unsubscribe.stop_notifications", { title: this.get("model.fancyTitle") });
}.property("model.fancyTitle"),
🤖 Prompt for AI Agents
In @app/assets/javascripts/discourse/controllers/topic-unsubscribe.js.es6 around
lines 5 - 7, Rename the misspelled method stopNotificiationsText to
stopNotificationsText in the controller (update the function name and its
property dependency string from "model.fancyTitle" as needed) and update the
template reference(s) in unsubscribe.hbs to use stopNotificationsText so both
sides match; search for any other usages of stopNotificiationsText and replace
them with stopNotificationsText to keep calls and bindings consistent.


})
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ export default function() {
this.route('fromParamsNear', { path: '/:nearPost' });
});
this.resource('topicBySlug', { path: '/t/:slug' });
this.route('topicUnsubscribe', { path: '/t/:slug/:id/unsubscribe' });

this.resource('discovery', { path: '/' }, function() {
// top
Expand Down
25 changes: 13 additions & 12 deletions app/assets/javascripts/discourse/routes/topic-from-params.js.es6
Original file line number Diff line number Diff line change
Expand Up @@ -2,32 +2,32 @@
export default Discourse.Route.extend({

// Avoid default model hook
model: function(p) { return p; },
model(params) { return params; },

setupController: function(controller, params) {
setupController(controller, params) {
params = params || {};
params.track_visit = true;
var topic = this.modelFor('topic'),
postStream = topic.get('postStream');

var topicController = this.controllerFor('topic'),
topicProgressController = this.controllerFor('topic-progress'),
composerController = this.controllerFor('composer');
const self = this,
topic = this.modelFor('topic'),
postStream = topic.get('postStream'),
topicController = this.controllerFor('topic'),
topicProgressController = this.controllerFor('topic-progress'),
composerController = this.controllerFor('composer');

// I sincerely hope no topic gets this many posts
if (params.nearPost === "last") { params.nearPost = 999999999; }

var self = this;
params.forceLoad = true;
postStream.refresh(params).then(function () {

postStream.refresh(params).then(function () {
// TODO we are seeing errors where closest post is null and this is exploding
// we need better handling and logging for this condition.

// The post we requested might not exist. Let's find the closest post
var closestPost = postStream.closestPostForPostNumber(params.nearPost || 1),
closest = closestPost.get('post_number'),
progress = postStream.progressIndexOfPost(closestPost);
const closestPost = postStream.closestPostForPostNumber(params.nearPost || 1),
closest = closestPost.get('post_number'),
progress = postStream.progressIndexOfPost(closestPost);

topicController.setProperties({
'model.currentPost': closest,
Expand All @@ -43,6 +43,7 @@ export default Discourse.Route.extend({
Ember.run.scheduleOnce('afterRender', function() {
self.appEvents.trigger('post:highlight', closest);
});

Discourse.URL.jumpToPost(closest);

if (topic.present('draft')) {
Expand Down
23 changes: 23 additions & 0 deletions app/assets/javascripts/discourse/routes/topic-unsubscribe.js.es6
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
import PostStream from "discourse/models/post-stream";

export default Discourse.Route.extend({
model(params) {
const topic = this.store.createRecord("topic", { id: params.id });
return PostStream.loadTopicView(params.id).then(json => {
topic.updateFromJson(json);
return topic;
});
},
Comment on lines +4 to +10

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

# Find and read the file
cat -n app/assets/javascripts/discourse/routes/topic-unsubscribe.js.es6

Repository: AI-Code-Review-Evals/coderabbit-discourse

Length of output: 817


🏁 Script executed:

# Search for other routes using loadTopicView to see error handling patterns
rg "loadTopicView" app/assets/javascripts/discourse/routes/ -A 5 -B 2

Repository: AI-Code-Review-Evals/coderabbit-discourse

Length of output: 823


🏁 Script executed:

# Look at the PostStream model to understand loadTopicView
fd "post-stream" app/assets/javascripts/discourse/models/ --type f

Repository: AI-Code-Review-Evals/coderabbit-discourse

Length of output: 143


🏁 Script executed:

# Find all route files and check error handling patterns
fd "\.es6$" app/assets/javascripts/discourse/routes/ --type f | head -10

Repository: AI-Code-Review-Evals/coderabbit-discourse

Length of output: 717


🏁 Script executed:

# Look at topic route specifically as it's likely related
cat -n app/assets/javascripts/discourse/routes/topic.js.es6 2>/dev/null || echo "File not found"

Repository: AI-Code-Review-Evals/coderabbit-discourse

Length of output: 8952


🏁 Script executed:

# Search for .catch patterns in routes to see error handling conventions
rg "\.catch|error:" app/assets/javascripts/discourse/routes/ -l

Repository: AI-Code-Review-Evals/coderabbit-discourse

Length of output: 201


🏁 Script executed:

# Check the PostStream.loadTopicView implementation
head -100 app/assets/javascripts/discourse/models/post-stream.js.es6

Repository: AI-Code-Review-Evals/coderabbit-discourse

Length of output: 3272


🏁 Script executed:

# Check topic-from-params.js.es6 for error handling patterns
cat -n app/assets/javascripts/discourse/routes/topic-from-params.js.es6

Repository: AI-Code-Review-Evals/coderabbit-discourse

Length of output: 2634


🏁 Script executed:

# Check user.js.es6 for error handling patterns
cat -n app/assets/javascripts/discourse/routes/user.js.es6 | head -150

Repository: AI-Code-Review-Evals/coderabbit-discourse

Length of output: 3230


🏁 Script executed:

# Search for any routes that use PostStream.loadTopicView and check their error handling
rg "PostStream\.loadTopicView" app/assets/javascripts/discourse/ -B 2 -A 10

Repository: AI-Code-Review-Evals/coderabbit-discourse

Length of output: 2465


Add error handling for loadTopicView promise rejection.

The promise returned by PostStream.loadTopicView is not handled for rejection. If the topic doesn't exist, user lacks permission, or a network error occurs, the rejected promise will propagate without proper error handling, leaving the user without feedback.

Other Discourse routes consistently handle such async operations with .catch() blocks (see user.js.es6 and topic-from-params.js.es6). Add a .catch() handler to redirect to an error page:

Proposed fix
   model(params) {
     const topic = this.store.createRecord("topic", { id: params.id });
     return PostStream.loadTopicView(params.id).then(json => {
       topic.updateFromJson(json);
       return topic;
+    }).catch(() => {
+      this.transitionTo("exception-unknown");
     });
   },
🤖 Prompt for AI Agents
In @app/assets/javascripts/discourse/routes/topic-unsubscribe.js.es6 around
lines 4 - 10, The model hook calls PostStream.loadTopicView without handling
rejections; add a .catch handler on the promise returned by
PostStream.loadTopicView(params.id) to handle errors, call topic.updateFromJson
in the .then as before, and in the .catch redirect the user to an appropriate
error route (e.g., this.transitionTo('not-found') or another centralized error
page) so failures (missing topic, permissions, network) are handled gracefully.


afterModel(topic) {
// hide the notification reason text
topic.set("details.notificationReasonText", null);
},

actions: {
didTransition() {
this.controllerFor("application").set("showFooter", true);
return true;
}
}
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
<div class="container">
<p>
{{{stopNotificiationsText}}}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Typo in property name and potential XSS concern with triple-brace rendering.

  1. Typo: stopNotificiationsText should be stopNotificationsText (extra 'i' in "Notificiations").

  2. Security: Triple braces {{{...}}} render unescaped HTML. If stopNotificationsText includes user-generated content like fancyTitle, this could introduce an XSS vulnerability. Ensure the title is properly escaped before interpolation, or use double braces if HTML rendering is not required.

🔒 Recommended fix

If HTML formatting is needed, ensure fancyTitle is sanitized in the controller. If not, use double braces:

-    {{{stopNotificiationsText}}}
+    {{stopNotificationsText}}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
{{{stopNotificiationsText}}}
{{stopNotificationsText}}
🤖 Prompt for AI Agents
In @app/assets/javascripts/discourse/templates/topic/unsubscribe.hbs at line 3,
The template uses a misspelled property name `stopNotificiationsText` — rename
it to `stopNotificationsText` in the template (and update any corresponding
references) and remove the triple-brace rendering to prevent XSS: replace the
`{{{...}}}` usage with safe rendering by using double braces
`{{stopNotificationsText}}` unless you explicitly need HTML; if HTML is
required, ensure the controller sanitizes `fancyTitle` (or other user-provided
parts) before composing `stopNotificationsText` so only trusted HTML is output.

</p>
<p>
{{i18n "topic.unsubscribe.change_notification_state"}} {{topic-notifications-button topic=model}}
</p>
</div>
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export default Discourse.View.extend({
classNames: ["topic-unsubscribe"]
});
Comment on lines +1 to +3

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

# Check prevalence of Discourse.View.extend in the codebase
rg -n "Discourse\.View\.extend" --type js | wc -l

Repository: AI-Code-Review-Evals/coderabbit-discourse

Length of output: 86


🏁 Script executed:

# Get a sample of Discourse.View usage with context
rg -n "Discourse\.View\.extend" --type js -A 1 | head -50

Repository: AI-Code-Review-Evals/coderabbit-discourse

Length of output: 520


🏁 Script executed:

# Check if there are any Ember components or other view patterns used
rg -n "Ember\.Component|\.extend\({" --type js | head -30

Repository: AI-Code-Review-Evals/coderabbit-discourse

Length of output: 204


🏁 Script executed:

# Search for Ember component patterns
rg -n "Ember\.Component\.extend" --type js | head -20

Repository: AI-Code-Review-Evals/coderabbit-discourse

Length of output: 836


🏁 Script executed:

# Check the structure of the views directory
find app/assets/javascripts -type d -name "views" 2>/dev/null

Repository: AI-Code-Review-Evals/coderabbit-discourse

Length of output: 158


🏁 Script executed:

# List files in the discourse views directory
ls -la app/assets/javascripts/discourse/views/ 2>/dev/null || echo "Directory may not exist yet"

Repository: AI-Code-Review-Evals/coderabbit-discourse

Length of output: 5967


🏁 Script executed:

# Check how views are typically structured - look at one of the existing admin views
cat app/assets/javascripts/admin/views/admin_user_view.js

Repository: AI-Code-Review-Evals/coderabbit-discourse

Length of output: 291


🏁 Script executed:

# Check what pattern other small discourse views use
head -10 app/assets/javascripts/discourse/views/badges-index.js.es6
head -10 app/assets/javascripts/discourse/views/badges-show.js.es6
head -10 app/assets/javascripts/discourse/views/unknown.js.es6
head -10 app/assets/javascripts/discourse/views/button.js.es6

Repository: AI-Code-Review-Evals/coderabbit-discourse

Length of output: 1173


🏁 Script executed:

# Check the file size and pattern of topic-unsubscribe.js.es6 
cat app/assets/javascripts/discourse/views/topic-unsubscribe.js.es6

Repository: AI-Code-Review-Evals/coderabbit-discourse

Length of output: 163


Reconsider if a View class is necessary for only setting classNames.

This view extends Discourse.View, which is consistently used throughout the codebase (both in admin and main views), but typically paired with mixins like ScrollTop or LoadMore. Since this view only sets classNames with no additional logic or mixins, it may be unnecessary to extend the View class—the CSS class could be applied at the template level instead.

🤖 Prompt for AI Agents
In @app/assets/javascripts/discourse/views/topic-unsubscribe.js.es6 around lines
1 - 3, This file exports a Discourse.View subclass that only sets classNames
("topic-unsubscribe"); remove the unnecessary view by deleting or not exporting
the Discourse.View.extend in topic-unsubscribe.js.es6 and instead apply the
"topic-unsubscribe" CSS class directly in the relevant template (or component)
where the view was used; if other modules import this symbol, replace those
imports/uses to reference the template/component with the class applied (or keep
a very small passthrough module that documents future expansion) so no empty
View subclass remains.

12 changes: 12 additions & 0 deletions app/assets/stylesheets/common/base/topic.scss
Original file line number Diff line number Diff line change
Expand Up @@ -63,3 +63,15 @@
// Top of bullet aligns with top of line - adjust line height to vertically align bullet.
line-height: 0.8;
}

.topic-unsubscribe {
.notification-options {
display: inline-block;
.dropdown-toggle {
float: none;
}
.dropdown-menu {
bottom: initial;
}
}
}
26 changes: 24 additions & 2 deletions app/controllers/topics_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,12 @@ class TopicsController < ApplicationController
:bulk,
:reset_new,
:change_post_owners,
:bookmark]
:bookmark,
:unsubscribe]

before_filter :consider_user_for_promotion, only: :show

skip_before_filter :check_xhr, only: [:show, :feed]
skip_before_filter :check_xhr, only: [:show, :unsubscribe, :feed]

def id_for_slug
topic = Topic.find_by(slug: params[:slug].downcase)
Expand Down Expand Up @@ -94,6 +95,26 @@ def show
raise ex
end

def unsubscribe
@topic_view = TopicView.new(params[:topic_id], current_user)

if slugs_do_not_match || (!request.format.json? && params[:slug].blank?)
return redirect_to @topic_view.topic.unsubscribe_url, status: 301
end

tu = TopicUser.find_by(user_id: current_user.id, topic_id: params[:topic_id])

if tu.notification_level > TopicUser.notification_levels[:regular]
tu.notification_level = TopicUser.notification_levels[:regular]
else
tu.notification_level = TopicUser.notification_levels[:muted]
end

tu.save!

perform_show_response
end
Comment on lines +98 to +116

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Potential NoMethodError if TopicUser record does not exist.

TopicUser.find_by returns nil when no record exists for the user/topic pair. This occurs when a user receives an email notification for a topic they haven't explicitly interacted with. Calling tu.notification_level on nil will raise a NoMethodError.

Consider using find_or_initialize_by or add a nil-check with appropriate fallback behavior (e.g., create a new record or redirect with an error).

Proposed fix using find_or_initialize_by
-  tu = TopicUser.find_by(user_id: current_user.id, topic_id: params[:topic_id])
+  tu = TopicUser.find_or_initialize_by(user_id: current_user.id, topic_id: params[:topic_id])
+  
+  # For new records, default to regular notification level
+  tu.notification_level ||= TopicUser.notification_levels[:regular]

   if tu.notification_level > TopicUser.notification_levels[:regular]
     tu.notification_level = TopicUser.notification_levels[:regular]
   else
     tu.notification_level = TopicUser.notification_levels[:muted]
   end

   tu.save!
🧰 Tools
🪛 RuboCop (1.82.1)

[convention] 98-116: Assignment Branch Condition size for unsubscribe is too high. [<4, 31, 6> 31.83/23]

(Metrics/AbcSize)

🤖 Prompt for AI Agents
In @app/controllers/topics_controller.rb around lines 98 - 116, The unsubscribe
action currently calls TopicUser.find_by which can return nil and cause a
NoMethodError when accessing tu.notification_level; change this to use
TopicUser.find_or_initialize_by(user_id: current_user.id, topic_id:
params[:topic_id]) (or explicitly nil-check tu and initialize a new TopicUser)
so tu is always an object before reading/writing notification_level, set the
appropriate default attributes on the initialized record (e.g., user_id,
topic_id) and then save! as currently done before calling perform_show_response.


def wordpress
params.require(:best)
params.require(:topic_id)
Expand Down Expand Up @@ -476,6 +497,7 @@ def perform_show_response
format.html do
@description_meta = @topic_view.topic.excerpt
store_preloaded("topic_#{@topic_view.topic.id}", MultiJson.dump(topic_view_serializer))
render :show
end

format.json do
Expand Down
5 changes: 2 additions & 3 deletions app/mailers/user_notifications.rb
Original file line number Diff line number Diff line change
Expand Up @@ -292,6 +292,7 @@ def send_notification_email(opts)
context: context,
username: username,
add_unsubscribe_link: true,
unsubscribe_url: post.topic.unsubscribe_url,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Potential NoMethodError if post.topic is nil.

If the topic has been deleted between the notification being created and the email being sent, post.topic will be nil, causing post.topic.unsubscribe_url to raise a NoMethodError.

Consider adding nil-safe navigation or a fallback:

Proposed fix
-      unsubscribe_url: post.topic.unsubscribe_url,
+      unsubscribe_url: post.topic&.unsubscribe_url,
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
unsubscribe_url: post.topic.unsubscribe_url,
unsubscribe_url: post.topic&.unsubscribe_url,
🤖 Prompt for AI Agents
In @app/mailers/user_notifications.rb at line 295, post.topic.unsubscribe_url
can raise NoMethodError if post.topic is nil; update the code that builds the
mail payload in UserNotifications (user_notifications.rb) to use nil-safe
navigation and a sensible fallback: get the topic into a local variable (e.g.,
topic = post.topic), set unsubscribe_url using safe navigation
(topic&.unsubscribe_url) and if nil supply a fallback URL or nil-safe value (for
example a generic unsubscribe path or a URL built from post.topic_id), and
ensure any helpers used (unsubscribe_url generator) handle a nil topic
gracefully.

allow_reply_by_email: allow_reply_by_email,
use_site_subject: use_site_subject,
add_re_to_subject: add_re_to_subject,
Expand All @@ -306,9 +307,7 @@ def send_notification_email(opts)
}

# If we have a display name, change the from address
if from_alias.present?
email_opts[:from_alias] = from_alias
end
email_opts[:from_alias] = from_alias if from_alias.present?

TopicUser.change(user.id, post.topic_id, last_emailed_post_number: post.post_number)

Expand Down
4 changes: 4 additions & 0 deletions app/models/topic.rb
Original file line number Diff line number Diff line change
Expand Up @@ -716,6 +716,10 @@ def relative_url(post_number=nil)
url
end

def unsubscribe_url
"#{url}/unsubscribe"
end

def clear_pin_for(user)
return unless user.present?
TopicUser.change(user.id, id, cleared_pinned_at: Time.now)
Expand Down
21 changes: 8 additions & 13 deletions app/models/topic_user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,9 @@ class TopicUser < ActiveRecord::Base

scope :tracking, lambda { |topic_id|
where(topic_id: topic_id)
.where("COALESCE(topic_users.notification_level, :regular) >= :tracking",
regular: TopicUser.notification_levels[:regular], tracking: TopicUser.notification_levels[:tracking])
.where("COALESCE(topic_users.notification_level, :regular) >= :tracking",
regular: TopicUser.notification_levels[:regular],
tracking: TopicUser.notification_levels[:tracking])
}

# Class methods
Expand Down Expand Up @@ -58,13 +59,9 @@ def lookup_for(user, topics)

def create_lookup(topic_users)
topic_users = topic_users.to_a

result = {}
return result if topic_users.blank?

topic_users.each do |ftu|
result[ftu.topic_id] = ftu
end
topic_users.each { |ftu| result[ftu.topic_id] = ftu }
result
end

Expand Down Expand Up @@ -113,11 +110,9 @@ def change(user_id, topic_id, attrs)
end

if attrs[:notification_level]
MessageBus.publish("/topic/#{topic_id}",
{notification_level_change: attrs[:notification_level]}, user_ids: [user_id])
MessageBus.publish("/topic/#{topic_id}", { notification_level_change: attrs[:notification_level] }, user_ids: [user_id])
end


rescue ActiveRecord::RecordNotUnique
# In case of a race condition to insert, do nothing
end
Expand All @@ -127,7 +122,7 @@ def track_visit!(topic,user)
user_id = user.is_a?(User) ? user.id : topic

now = DateTime.now
rows = TopicUser.where({topic_id: topic_id, user_id: user_id}).update_all({last_visited_at: now})
rows = TopicUser.where(topic_id: topic_id, user_id: user_id).update_all(last_visited_at: now)
if rows == 0
TopicUser.create(topic_id: topic_id, user_id: user_id, last_visited_at: now, first_visited_at: now)
else
Expand Down Expand Up @@ -196,7 +191,7 @@ def update_last_read(user, topic_id, post_number, msecs, opts={})
end

if before != after
MessageBus.publish("/topic/#{topic_id}", {notification_level_change: after}, user_ids: [user.id])
MessageBus.publish("/topic/#{topic_id}", { notification_level_change: after }, user_ids: [user.id])
end
end

Expand All @@ -220,7 +215,7 @@ def update_last_read(user, topic_id, post_number, msecs, opts={})
WHERE ftu.user_id = :user_id and ftu.topic_id = :topic_id)",
args)

MessageBus.publish("/topic/#{topic_id}", {notification_level_change: args[:new_status]}, user_ids: [user.id])
MessageBus.publish("/topic/#{topic_id}", { notification_level_change: args[:new_status] }, user_ids: [user.id])
end
end

Expand Down
40 changes: 19 additions & 21 deletions app/views/email/notification.html.erb
Original file line number Diff line number Diff line change
@@ -1,31 +1,29 @@
<div id='main' class=<%= classes %>>

<%= render :partial => 'email/post', :locals => {:post => post} %>
<%= render partial: 'email/post', locals: { post: post } %>

<% if context_posts.present? %>
<div class='footer'>
%{respond_instructions}
</div>
<hr>
<h4 class='.previous-discussion'><%= t "user_notifications.previous_discussion" %></h4>
<% if context_posts.present? %>
<div class='footer'>%{respond_instructions}</div>

<hr>

<% context_posts.each do |p| %>
<%= render :partial => 'email/post', :locals => {:post => p} %>
<h4 class='.previous-discussion'><%= t "user_notifications.previous_discussion" %></h4>

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Typo in class attribute: remove leading dot.

Class names in HTML attributes should not include the leading dot (.). The dot notation is only used in CSS selectors. This will cause the CSS styles targeting .previous-discussion to not apply.

Proposed fix
-    <h4 class='.previous-discussion'><%= t "user_notifications.previous_discussion" %></h4>
+    <h4 class='previous-discussion'><%= t "user_notifications.previous_discussion" %></h4>
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
<h4 class='.previous-discussion'><%= t "user_notifications.previous_discussion" %></h4>
<h4 class='previous-discussion'><%= t "user_notifications.previous_discussion" %></h4>
🤖 Prompt for AI Agents
In @app/views/email/notification.html.erb at line 10, The h4 element currently
uses an invalid class attribute value '.previous-discussion' which includes a
leading dot; update the class on the h4 (the element rendering t
"user_notifications.previous_discussion") to remove the dot so it reads
class="previous-discussion" so CSS selectors targeting .previous-discussion will
apply correctly.


<% context_posts.each do |p| %>
<%= render partial: 'email/post', locals: { post: p } %>
<% end %>
<% end %>
<% end %>

<hr>
<hr>

<div class='footer'>%{respond_instructions}</div>
<div class='footer'>%{unsubscribe_link}</div>

<div class='footer'>
%{respond_instructions}
</div>
<div class='footer'>
%{unsubscribe_link}
</div>
</div>

<div itemscope itemtype="http://schema.org/EmailMessage" style="display:none">
<div itemprop="action" itemscope itemtype="http://schema.org/ViewAction">
<link itemprop="url" href="<%= Discourse.base_url %><%= post.url %>" />
<meta itemprop="name" content="<%= t 'read_full_topic' %>"/>
</div>
<div itemprop="action" itemscope itemtype="http://schema.org/ViewAction">
<link itemprop="url" href="<%= Discourse.base_url %><%= post.url %>" />
<meta itemprop="name" content="<%= t 'read_full_topic' %>"/>
</div>
</div>
4 changes: 3 additions & 1 deletion config/locales/client.en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -981,6 +981,9 @@ en:
search: "There are no more search results."

topic:
unsubscribe:
stop_notifications: "You will stop receiving notifications for <strong>{{title}}</strong>."
change_notification_state: "You can change your notification state"
filter_to: "{{post_count}} posts in topic"
Comment on lines 983 to 987

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Search for usage of these translation keys
rg -n "topic\.unsubscribe\.stop_notifications|topic\.unsubscribe\.change_notification_state" --type js --type ts --type handlebars -B 2 -A 2

Repository: AI-Code-Review-Evals/coderabbit-discourse

Length of output: 226


🏁 Script executed:

#!/bin/bash
# Search for "stop_notifications" in the entire codebase to find rendering context
rg -i "stop_notifications" --type js --type ts --type html --type handlebars -B 3 -A 3

Repository: AI-Code-Review-Evals/coderabbit-discourse

Length of output: 119


🏁 Script executed:

#!/bin/bash
# Check for i18n/translation rendering patterns that might indicate html-safe behavior
rg -n "html-safe|htmlSafe|\.html\(|triple.*mustache|{{{" app/assets -A 2 -B 2 | head -50

Repository: AI-Code-Review-Evals/coderabbit-discourse

Length of output: 253


🏁 Script executed:

#!/bin/bash
# Look for the topic unsubscribe notification logic
fd -e js -e ts -e hbs | xargs grep -l "unsubscribe" | head -10

Repository: AI-Code-Review-Evals/coderabbit-discourse

Length of output: 232


🏁 Script executed:

cat -n app/assets/javascripts/discourse/templates/topic/unsubscribe.hbs

Repository: AI-Code-Review-Evals/coderabbit-discourse

Length of output: 332


🏁 Script executed:

rg -n "stop_notifications" app/assets/javascripts -A 5 -B 5

Repository: AI-Code-Review-Evals/coderabbit-discourse

Length of output: 1013


🏁 Script executed:

# Search for i18n helper usage and escaping behavior
rg -n "i18n|t\(" app/assets/javascripts/discourse/templates/topic/unsubscribe.hbs -A 2 -B 2

Repository: AI-Code-Review-Evals/coderabbit-discourse

Length of output: 223


🏁 Script executed:

# Check for any js that handles the unsubscribe view/controller
fd -e js -e ts app/assets/javascripts | xargs grep -l "unsubscribe" 2>/dev/null | head -5

Repository: AI-Code-Review-Evals/coderabbit-discourse

Length of output: 478


🏁 Script executed:

# Look for i18n/translation rendering patterns - check how handlebars renders translations
rg -n "interpolate|htmlSafe|\.html" app/assets/javascripts/discourse -A 1 -B 1 | grep -i "i18n\|t\(" | head -20

Repository: AI-Code-Review-Evals/coderabbit-discourse

Length of output: 108


🏁 Script executed:

# Find the I18n library implementation
fd -e js "i18n" app/ vendor/ public/ | grep -i i18n | head -10

Repository: AI-Code-Review-Evals/coderabbit-discourse

Length of output: 165


🏁 Script executed:

# Search for I18n.t implementation to see if it escapes interpolated values
rg -n "I18n\s*=|\.t\s*=|function t\(" -A 10 vendor/assets/javascripts/ --type js | head -50

Repository: AI-Code-Review-Evals/coderabbit-discourse

Length of output: 67


🏁 Script executed:

# Look for discourse's i18n helper or how interpolation works
rg -n "interpolate" app/assets/javascripts -A 5 | head -30

Repository: AI-Code-Review-Evals/coderabbit-discourse

Length of output: 1369


🏁 Script executed:

# Check if there's any escaping of interpolated values in i18n
rg -n "escape|htmlEscape" vendor/assets/javascripts/ --type js -B 2 -A 2 | head -30

Repository: AI-Code-Review-Evals/coderabbit-discourse

Length of output: 2416


🏁 Script executed:

cat -n app/assets/javascripts/locales/i18n.js | sed -n '167,200p'

Repository: AI-Code-Review-Evals/coderabbit-discourse

Length of output: 1297


🏁 Script executed:

# Get more of the interpolate function
sed -n '167,190p' app/assets/javascripts/locales/i18n.js

Repository: AI-Code-Review-Evals/coderabbit-discourse

Length of output: 688


🏁 Script executed:

# Look at the full interpolate function
awk '/I18n\.interpolate = function/,/^I18n\./ {print NR": "$0}' app/assets/javascripts/locales/i18n.js | head -40

Repository: AI-Code-Review-Evals/coderabbit-discourse

Length of output: 137


🏁 Script executed:

# Check if there are any utility functions for escaping in Discourse
rg -n "Handlebars\.Utils\.escapeExpression|escape|htmlEscape" app/assets/javascripts -A 2 -B 1 | head -30

Repository: AI-Code-Review-Evals/coderabbit-discourse

Length of output: 2621


🏁 Script executed:

# Verify what fancyTitle contains and where it comes from
rg -n "fancyTitle" app/assets/javascripts -B 2 -A 2 | head -20

Repository: AI-Code-Review-Evals/coderabbit-discourse

Length of output: 1781


HTML injection vulnerability: {{title}} must be escaped before rendering as HTML.

The translation string contains <strong>{{title}}</strong> and is rendered via {{{stopNotificiationsText}}} (triple braces = HTML-safe). The controller passes model.fancyTitle (user-controlled topic title) to I18n.t(), but I18n.interpolate() performs direct string replacement without HTML escaping (line 188 in i18n.js). This allows attackers to inject HTML/JavaScript via the topic title.

Escaping must occur before interpolation or the rendered output will be unsafe.

🤖 Prompt for AI Agents
In @config/locales/client.en.yml around lines 983 - 987, The translation uses
raw HTML with <strong>{{title}}</strong> and is rendered with triple-brace
stopNotificiationsText, while I18n.interpolate() (used by I18n.t()) does not
escape values; fix by ensuring model.fancyTitle is HTML-escaped before
interpolation (or update I18n.interpolate() to escape interpolation values by
default) so unsafe input cannot inject HTML/JS; update the controller that
supplies model.fancyTitle (or the I18n.t() call that produces
stopNotificiationsText) to pass an escaped title, or change the interpolation
implementation to escape values prior to replacing {{title}}.

create: 'New Topic'
create_long: 'Create a new Topic'
Expand Down Expand Up @@ -1014,7 +1017,6 @@ en:
new_posts:
one: "there is 1 new post in this topic since you last read it"
other: "there are {{count}} new posts in this topic since you last read it"

likes:
one: "there is 1 like in this topic"
other: "there are {{count}} likes in this topic"
Expand Down
5 changes: 4 additions & 1 deletion config/locales/server.en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -1849,7 +1849,10 @@ en:
subject_template: "Downloading remote images disabled"
text_body_template: "The `download_remote_images_to_local` setting was disabled because the disk space limit at `download_remote_images_threshold` was reached."

unsubscribe_link: "To unsubscribe from these emails, visit your [user preferences](%{user_preferences_url})."
unsubscribe_link: |
To unsubscribe from these emails, visit your [user preferences](%{user_preferences_url}).

To stop receiving notifications about this particular topic, [click here](%{unsubscribe_url}).

subject_re: "Re: "
subject_pm: "[PM] "
Expand Down
8 changes: 5 additions & 3 deletions config/routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -434,10 +434,12 @@
# Topic routes
get "t/id_for/:slug" => "topics#id_for_slug"
get "t/:slug/:topic_id/wordpress" => "topics#wordpress", constraints: {topic_id: /\d+/}
get "t/:slug/:topic_id/moderator-liked" => "topics#moderator_liked", constraints: {topic_id: /\d+/}
get "t/:topic_id/wordpress" => "topics#wordpress", constraints: {topic_id: /\d+/}
get "t/:slug/:topic_id/summary" => "topics#show", defaults: {summary: true}, constraints: {topic_id: /\d+/, post_number: /\d+/}
get "t/:topic_id/summary" => "topics#show", constraints: {topic_id: /\d+/, post_number: /\d+/}
get "t/:slug/:topic_id/moderator-liked" => "topics#moderator_liked", constraints: {topic_id: /\d+/}
get "t/:slug/:topic_id/summary" => "topics#show", defaults: {summary: true}, constraints: {topic_id: /\d+/}
get "t/:slug/:topic_id/unsubscribe" => "topics#unsubscribe", constraints: {topic_id: /\d+/}
get "t/:topic_id/unsubscribe" => "topics#unsubscribe", constraints: {topic_id: /\d+/}
get "t/:topic_id/summary" => "topics#show", constraints: {topic_id: /\d+/}
put "t/:slug/:topic_id" => "topics#update", constraints: {topic_id: /\d+/}
put "t/:slug/:topic_id/star" => "topics#star", constraints: {topic_id: /\d+/}
put "t/:topic_id/star" => "topics#star", constraints: {topic_id: /\d+/}
Expand Down
Loading