Benchmark / Discourse / PR #2

FEATURE: per-topic unsubscribe option in emails

marchugon/discourse-cursor-fr-benchmark#6
Golden recall: 50% Extra findings: 7

Golden Comments (1/2 found)

# Comment Severity Status FriendlyReviewer Detail
#1 Nil pointer in topics_controller.rb: tu = TopicUser.find_by(user_id:..., topic_id:...) can return nil if no TopicUser exists, then tu.notification_level and tu.save! crash (NoMethodError) HIGH ✓ Found FriendlyReviewer: "CRITICAL BUG: TopicUser.find_by(...) can return nil, causing NoMethodError on nil.notification_level. Use TopicUser.find_or_create_by or handle the nil case explicitly." (topics_controller.rb L107)
#2 Typo in topic-unsubscribe.js.es6: stopNotificiationsText (extra "i" after "t") instead of stopNotificationsText LOW ✗ Missed FriendlyReviewer did not flag the JavaScript typo.

Supplementary Findings (10 findings)

Finding File Severity Legitimate?
Missing error handling for TopicView.new — crash with 500 if topic is deleted, private, or inaccessible topics_controller.rb:95 HIGH ✓ Production crash scenario
No user-facing feedback after changing notification levels — add flash notice or dedicated template topics_controller.rb:95 MEDIUM ✓ Missing user feedback
Notification level toggle is progressive (tracking → regular), not directly to muted — may not match user expectations of a true unsubscribe topics_controller.rb:110 MEDIUM ✓ May not match user expectations
tu.save! raises ActiveRecord::RecordInvalid without rescue — use non-bang save with conditional handling topics_controller.rb:114 MEDIUM ✓ RecordInvalid without rescue
Test adds unsubscribe_url to builder opts but never asserts its presence in output (e.g., template_args) message_builder_spec.rb:172 MEDIUM ✓ Missing assertion
Test validates body contains user preferences URL but does not verify the new per-topic unsubscribe_url is present message_builder_spec.rb:181 MEDIUM ✓ Missing assertion for new URL
No tests for the HTML part of the email — the per-topic unsubscribe link is rendered in HTML only message_builder_spec.rb:168 MEDIUM ✓ HTML part untested
CSS styles for .topic-unsubscribe may be dead code if no corresponding JavaScript component exists topic.scss:66 LOW ⚠ Could be dead code
Test uses builder.template_args[:user_preferences_url] which refers to a different instance — prefer using the instance under test message_builder_spec.rb:181 LOW 💅 Minor style issue
List-Unsubscribe header test only checks be_present, not correctness — also consider adding per-topic unsubscribe_url per RFC 2369 message_builder_spec.rb:174 LOW 💅 Weak assertion

Honest Analysis

FriendlyReviewer detected 1 of 2 golden bugs in this PR. The critical nil-pointer bug in topics_controller.rb (#1) was identified with a precise diagnosis and a clear fix suggestion — FriendlyReviewer correctly flagged that TopicUser.find_by(...) can return nil, causing a NoMethodError on nil.notification_level. The minor typo in topic-unsubscribe.js.es6 (stopNotificiationsText) was missed — this is a cosmetic variable naming issue with no functional impact, and it is acceptable for a reviewer to skip it.

The 2 HIGH findings from FriendlyReviewer — the missing error handling for TopicView.new and the nil-pointer bug itself — are both legitimate and address production-relevant crash scenarios. The 6 YELLOW findings cover UX concerns (no flash feedback after unsubscribe, progressive toggle semantics that may confuse users), error handling (save! without rescue), and incomplete test coverage. All are valid observations.

The noise level is low. The CSS dead code finding (.topic-unsubscribe) is a reasonable question in a code review context — if the CSS has no JavaScript consumer, it should either be used or removed. The fragile test assertions (cross-instance template_args access, List-Unsubscribe header checked with only be_present) are minor style issues that do not detract from the overall value of the review.

FriendlyReviewer's analysis covers the controller logic, the email pipeline, test coverage, and CSS — a broad and useful review. The only gap is the JavaScript typo, which is a variable naming issue more suited to a linter than a code reviewer.

FriendlyReviewer's full review

As posted on the PR — the complete output from FriendlyReviewer.

friendly-reviewer bot commented now

Hi there 👋

🌥️ Tech
2 high, 6 medium, 3 low

The MR introduces a per-topic unsubscribe link in email notifications. The core implementation is correct: the Topic#unsubscribe_url method, SCSS styles, email plumbing through MessageBuilder, and route definitions are all well-designed. However, critical issues in the TopicsController#unsubscribe action make the feature unsafe for production. The action lacks nil handling for TopicUser records, proper exception handling for TopicView.new, and user-facing feedback. Additionally, test coverage for the email pipeline is incomplete, failing to verify the new unsubscribe_url appears in output. The notification level toggle logic may also confuse users expecting a one-click unsubscribe. These issues must be addressed before merging.

These might need a close look

  • 🔴 app/controllers/topics_controller.rb (L95)
    Missing error handling for TopicView.new. Unlike show, this action will crash with a 500 error if the topic is deleted, private, or inaccessible. Add rescue blocks for Discourse::NotFound and Discourse::InvalidAccess.
  • 🔴 app/controllers/topics_controller.rb (L107)
    CRITICAL BUG: TopicUser.find_by(...) can return nil, causing NoMethodError on nil.notification_level. Use TopicUser.find_or_create_by or handle the nil case explicitly.

Worth checking

  • 🟡 app/controllers/topics_controller.rb (L95)
    No user-facing feedback after changing notification levels. Add a flash notice or dedicated template to confirm the unsubscribe action.
  • 🟡 app/controllers/topics_controller.rb (L110)
    Notification level toggle is progressive: from tracking/watching to regular, not directly to muted. This may not match user expectations of a true 'unsubscribe'. Consider going directly to muted when invoked from an unsubscribe link.
  • 🟡 app/controllers/topics_controller.rb (L114)
    tu.save! raises ActiveRecord::RecordInvalid without rescue. Use non-bang save with conditional handling or add error handling.
  • 🟡 spec/components/email/message_builder_spec.rb (L172)
    Test adds unsubscribe_url to builder opts but never asserts its presence in output (e.g., template_args). Add assertion to verify inclusion.
  • 🟡 spec/components/email/message_builder_spec.rb (L181)
    Test validates body contains user preferences URL but does not verify the per-topic unsubscribe_url is present. Add assertion for the new URL.
  • 🟡 spec/components/email/message_builder_spec.rb (L168)
    No tests for the HTML part of the email. The per-topic unsubscribe link is rendered in HTML; add tests to verify both URLs appear in html_part output.

Small things (take or leave)

  • 🔵 app/assets/stylesheets/common/base/topic.scss (L66)
    CSS styles for .topic-unsubscribe may be dead code if no corresponding JavaScript component exists. Confirm client-side usage exists.
  • 🔵 spec/components/email/message_builder_spec.rb (L181)
    Test uses builder.template_args[:user_preferences_url] which refers to a different instance; prefer using the instance under test or literal strings.
  • 🔵 spec/components/email/message_builder_spec.rb (L174)
    List-Unsubscribe header test only checks be_present, not correctness. Also consider adding per-topic unsubscribe_url to the header per RFC 2369.