FEATURE: per-topic unsubscribe option in emails#2
Conversation
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughIntroduces a topic unsubscribe feature enabling users to disable notifications for specific topics via a dedicated unsubscribe page. Adds frontend routes, controllers, views, and templates; backend controller action and model method; email integration updates; localization strings; and test coverage. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Email
participant Client as Browser/Client
participant Route as Route Handler
participant Controller as Topics Controller
participant Model as Topic/TopicUser
participant DB as Database
User->>Email: Receives notification with unsubscribe link
User->>Client: Clicks unsubscribe link
Client->>Route: GET /t/:slug/:id/unsubscribe
Route->>Route: Load topic view data via PostStream
Route->>Controller: setupController with topic
Controller->>Model: Get topic details
Model->>DB: Query topic and notification state
DB-->>Model: Return topic data
Controller->>Client: Render unsubscribe confirmation page
User->>Client: Submit notification preference change
Client->>Controller: POST unsubscribe action
Controller->>Model: Toggle TopicUser notification_level
Model->>DB: Update notification setting
DB-->>Model: Confirm update
Controller->>Client: Redirect to show response
Client-->>User: Display updated topic
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/assets/javascripts/discourse/routes/app-route-map.js.es6 (1)
8-13: MovetopicUnsubscriberoute definition inside thetopicresource as a nested route.The standalone
topicUnsubscriberoute at/t/:slug/:id/unsubscribewon't be reachable because thetopicresource (matching/t/:slug/:id) will intercept the request first. In Ember's router, the resource only recognizes its explicitly defined nested routes (fromParamsandfromParamsNear), so a request to/unsubscribewould have no matching nested route and fail to route correctly.Define
topicUnsubscribeinside thetopicresource's function block instead:Suggested fix
this.resource('topic', { path: '/t/:slug/:id' }, function() { this.route('fromParams', { path: '/' }); this.route('fromParamsNear', { path: '/:nearPost' }); this.route('unsubscribe', { path: '/unsubscribe' }); });
🤖 Fix all issues with AI agents
In @app/assets/javascripts/discourse/controllers/topic-unsubscribe.js.es6:
- Around line 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.
In @app/assets/javascripts/discourse/routes/topic-unsubscribe.js.es6:
- Around line 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.
In @app/assets/javascripts/discourse/templates/topic/unsubscribe.hbs:
- 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.
In @app/assets/javascripts/discourse/views/topic-unsubscribe.js.es6:
- Around line 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.
In @app/controllers/topics_controller.rb:
- Around line 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.
In @app/mailers/user_notifications.rb:
- 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.
In @app/views/email/notification.html.erb:
- 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.
In @config/locales/client.en.yml:
- Around line 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}}.
🧹 Nitpick comments (4)
app/models/topic_user.rb (1)
60-66:create_lookupcan be simplified withindex_by, if you want.
Current code is fine; optional readability tweak:topic_users.to_a.index_by(&:topic_id).lib/email/message_builder.rb (1)
61-68: Address RuboCop warning: assignment in condition.Line 62 uses assignment within the
ifcondition. While this is a valid Ruby idiom, wrapping it in parentheses clarifies intent and silences the linter warning.Proposed fix
- if response_instructions = @template_args[:respond_instructions] + if (response_instructions = @template_args[:respond_instructions])spec/components/email/message_builder_spec.rb (1)
167-182: LGTM, but consider adding test coverage forunsubscribe_urlusage.The test correctly adds the
unsubscribe_urlparameter, but only verifies the existingList-Unsubscribeheader anduser_preferences_urlin body behavior. Consider adding an assertion that theunsubscribe_urlvalue is actually included in the template_args or rendered output.it "includes the unsubscribe_url in template_args" do expect(message_with_unsubscribe.template_args[:unsubscribe_url]).to eq("/t/1234/unsubscribe") endapp/controllers/topics_controller.rb (1)
27-28: Add trailing comma after the last array element.Per RuboCop style convention, multiline arrays should have a trailing comma after the last element for cleaner diffs.
Suggested fix
:bookmark, - :unsubscribe] + :unsubscribe,]
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
app/assets/javascripts/discourse/components/dropdown-button.js.es6app/assets/javascripts/discourse/controllers/topic-unsubscribe.js.es6app/assets/javascripts/discourse/routes/app-route-map.js.es6app/assets/javascripts/discourse/routes/topic-from-params.js.es6app/assets/javascripts/discourse/routes/topic-unsubscribe.js.es6app/assets/javascripts/discourse/templates/topic/unsubscribe.hbsapp/assets/javascripts/discourse/views/topic-unsubscribe.js.es6app/assets/stylesheets/common/base/topic.scssapp/controllers/topics_controller.rbapp/mailers/user_notifications.rbapp/models/topic.rbapp/models/topic_user.rbapp/views/email/notification.html.erbconfig/locales/client.en.ymlconfig/locales/server.en.ymlconfig/routes.rblib/email/message_builder.rbspec/components/email/message_builder_spec.rb
🧰 Additional context used
🧬 Code graph analysis (5)
app/models/topic.rb (4)
app/models/post.rb (2)
url(342-348)url(350-352)app/models/category.rb (1)
url(392-404)app/serializers/similar_topic_serializer.rb (1)
url(14-16)app/models/notification.rb (1)
url(90-94)
app/mailers/user_notifications.rb (2)
app/models/notification.rb (1)
post(96-100)app/models/topic.rb (1)
unsubscribe_url(719-721)
app/models/topic_user.rb (1)
lib/distributed_cache.rb (1)
publish(64-69)
app/controllers/topics_controller.rb (3)
app/models/topic.rb (1)
unsubscribe_url(719-721)app/serializers/listable_topic_serializer.rb (1)
notification_level(64-66)app/models/topic_user.rb (1)
notification_levels(19-21)
lib/email/message_builder.rb (1)
lib/discourse.rb (1)
base_url(211-213)
🪛 RuboCop (1.82.1)
app/controllers/topics_controller.rb
[convention] 28-28: Put a comma after the last item of a multiline array.
(Style/TrailingCommaInArrayLiteral)
[convention] 98-116: Assignment Branch Condition size for unsubscribe is too high. [<4, 31, 6> 31.83/23]
(Metrics/AbcSize)
lib/email/message_builder.rb
[warning] 62-62: Use == if you meant to do a comparison or wrap the expression in parentheses to indicate you meant to assign in a condition.
(Lint/AssignmentInCondition)
🔇 Additional comments (17)
app/assets/javascripts/discourse/components/dropdown-button.js.es6 (1)
27-30: Conditional rendering logic is correct.The guard prevents rendering an empty
<h4>element when no title is provided, which is a sensible improvement. Bothnotifications-buttonandpinned-buttoncomponents (the only subclasses ofDropdownButton) settitle: ''and never modify it, so this conditional will prevent unnecessary empty elements.Note: The direct string concatenation pattern exists elsewhere in this file as well, but it is pre-existing and not introduced by this change.
app/models/topic_user.rb (3)
8-13: Scope formatting is fine; keep the chained.wherereadable.
No semantic concerns here.
112-114: MessageBus publishes are consistent across all locations.All three occurrences in topic_user.rb (lines 112–114, 193–195, 218–219) publish with the same payload structure:
{ notification_level_change: ... }. JavaScript subscribers in topic.js.es6 (lines 585–591) correctly subscribe to/topic/:idand expect thenotification_level_changekey, confirming the client expects this payload shape.
120-131: I cannot verify this review comment because the repository is inaccessible. While the code snippet shown contains what appears to be a clear logic error on line 122 (assigningtopicinstead ofusertouser_id), I cannot confirm:
- Whether this is the actual current code in the repository
- How the method is called and what argument types are passed
- Whether this bug path is exercised in practice
- The actual impact or severity
Manual verification of the codebase and call sites is required to definitively assess this issue.
app/models/topic.rb (1)
719-721: The implementation is correct. The route shape is exactlyt/:slug/:topic_id/unsubscribeandt/:topic_id/unsubscribe(per config/routes.rb), andTopic#unsubscribe_urlcorrectly expands to the full URL via#{url}/unsubscribe. No token or query parameter is needed—the unsubscribe action relies on the current user context. The method is properly used in mailer notifications and handles absolute URLs appropriately for emails.Likely an incorrect or invalid review comment.
config/locales/server.en.yml (1)
1852-1856: No email breakage risk—the only call site already passesunsubscribe_url.The concern about missing
unsubscribe_urlinterpolation is unfounded. A single call site inapp/mailers/user_notifications.rb(line 294-295) setsadd_unsubscribe_link: trueand immediately providesunsubscribe_url: post.topic.unsubscribe_urlin the same options block. No other mailers useadd_unsubscribe_link, and rejection mailer explicitly disallows it. This was a coordinated feature release ("FEATURE: per-topic unsubscribe option in emails"), so the i18n key and call sites were updated together.Likely an incorrect or invalid review comment.
app/assets/javascripts/discourse/routes/topic-from-params.js.es6 (2)
5-16: LGTM! Clean refactor to ES6 conventions.The updates to use ES6 shorthand method syntax and
constdeclarations improve code clarity. Theconst self = thispattern is appropriate here for the.then()callback context.
28-30: LGTM!Converting post-lookup variables to
constis appropriate since these values are not reassigned.app/assets/stylesheets/common/base/topic.scss (1)
67-77: LGTM!The scoped styles appropriately customize the notification dropdown behavior within the unsubscribe context. The
float: noneandbottom: initialoverrides ensure proper layout for the dropdown in this specific view.app/assets/javascripts/discourse/templates/topic/unsubscribe.hbs (1)
6-6: LGTM!The translation helper and component binding are correctly implemented.
app/mailers/user_notifications.rb (1)
310-310: LGTM!Clean refactor from multi-line conditional to a single-line assignment with guard clause.
lib/email/message_builder.rb (2)
24-28: LGTM!Clean formatting with trailing comma—follows Ruby best practices for multi-line hashes and makes future additions cleaner.
34-38: LGTM!The refactored conditional assignment is cleaner while preserving the original behavior.
app/assets/javascripts/discourse/routes/topic-unsubscribe.js.es6 (1)
12-15: LGTM!The
afterModelhook appropriately clears notification text for the unsubscribe context, anddidTransitioncorrectly enables the footer.Also applies to: 17-22
app/controllers/topics_controller.rb (1)
500-500: LGTM!Explicitly rendering
:showaligns with the preloaded data stored on the previous line, ensuring the HTML response correctly uses the serialized topic view.app/views/email/notification.html.erb (1)
5-6: Duplicate footer placement may cause confusion.The
respond_instructionsfooter appears twice: once inside thecontext_posts.present?block (line 6) and once outside it (line 19). This means when context posts are present, users will see the respond instructions twice in the email. Verify this is the intended behavior.Also applies to: 17-20
config/routes.rb (1)
438-442: LGTM!The new routes follow existing conventions with appropriate constraints and provide both slugged and non-slugged variants for the unsubscribe endpoint, consistent with other topic routes in this file.
| stopNotificiationsText: function() { | ||
| return I18n.t("topic.unsubscribe.stop_notifications", { title: this.get("model.fancyTitle") }); | ||
| }.property("model.fancyTitle"), |
There was a problem hiding this comment.
🧩 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" -nRepository: 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.
| 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.
| model(params) { | ||
| const topic = this.store.createRecord("topic", { id: params.id }); | ||
| return PostStream.loadTopicView(params.id).then(json => { | ||
| topic.updateFromJson(json); | ||
| return topic; | ||
| }); | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find and read the file
cat -n app/assets/javascripts/discourse/routes/topic-unsubscribe.js.es6Repository: 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 2Repository: 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 fRepository: 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 -10Repository: 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/ -lRepository: 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.es6Repository: 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.es6Repository: 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 -150Repository: 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 10Repository: 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.
| @@ -0,0 +1,8 @@ | |||
| <div class="container"> | |||
| <p> | |||
| {{{stopNotificiationsText}}} | |||
There was a problem hiding this comment.
Typo in property name and potential XSS concern with triple-brace rendering.
-
Typo:
stopNotificiationsTextshould bestopNotificationsText(extra 'i' in "Notificiations"). -
Security: Triple braces
{{{...}}}render unescaped HTML. IfstopNotificationsTextincludes user-generated content likefancyTitle, 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.
| {{{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.
| export default Discourse.View.extend({ | ||
| classNames: ["topic-unsubscribe"] | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check prevalence of Discourse.View.extend in the codebase
rg -n "Discourse\.View\.extend" --type js | wc -lRepository: 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 -50Repository: 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 -30Repository: 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 -20Repository: 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/nullRepository: 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.jsRepository: 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.es6Repository: 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.es6Repository: 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.
| 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 |
There was a problem hiding this comment.
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.
| context: context, | ||
| username: username, | ||
| add_unsubscribe_link: true, | ||
| unsubscribe_url: post.topic.unsubscribe_url, |
There was a problem hiding this comment.
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.
| 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.
|
|
||
| <% context_posts.each do |p| %> | ||
| <%= render :partial => 'email/post', :locals => {:post => p} %> | ||
| <h4 class='.previous-discussion'><%= t "user_notifications.previous_discussion" %></h4> |
There was a problem hiding this comment.
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.
| <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.
| 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" |
There was a problem hiding this comment.
🧩 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 2Repository: 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 3Repository: 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 -50Repository: 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 -10Repository: AI-Code-Review-Evals/coderabbit-discourse
Length of output: 232
🏁 Script executed:
cat -n app/assets/javascripts/discourse/templates/topic/unsubscribe.hbsRepository: AI-Code-Review-Evals/coderabbit-discourse
Length of output: 332
🏁 Script executed:
rg -n "stop_notifications" app/assets/javascripts -A 5 -B 5Repository: 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 2Repository: 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 -5Repository: 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 -20Repository: 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 -10Repository: 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 -50Repository: 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 -30Repository: 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 -30Repository: 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.jsRepository: 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 -40Repository: 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 -30Repository: 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 -20Repository: 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}}.
2 participants
Test 2
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.
Replicated from ai-code-review-evaluation/discourse-coderabbit#2