Add Slack mute button and mute notification suppression#1
Open
j4rs wants to merge 1 commit into
Open
Conversation
- Gemfile points to j4rs/rails_error_dashboard fork (mute feature branch) - Slack notification now includes Mute button alongside Resolve - SlackInteractionsController handles mute_error action - Clicking Mute records who muted it and replaces button with label - Integration spec verifies muted errors don't trigger Slack jobs - Request spec covers Slack mute interaction with signature verification Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
WalkthroughThis pull request adds error muting functionality to the Rails error dashboard integration. The Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 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📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/controllers/slack_interactions_controller.rb`:
- Around line 86-102: The duplicated flat_map block-manipulation in mute_error
and resolve_error should be extracted into a private helper (e.g.,
update_action_blocks) to reduce duplication and cyclomatic complexity; implement
update_action_blocks(original_blocks, filter_action_prefix:, context_text:) that
performs the flat_map, rejects elements by action_id prefix and appends a
context block, then replace the inline logic in both mute_error and
resolve_error to call this helper passing "mute_error_" / "resolve_error_" (or
appropriate prefixes) and the respective context_text (e.g., ":bell_slash:
*Muted* by #{user_mention}").
- Around line 79-81: Wrap the call to
RailsErrorDashboard::Commands::MuteError.call(error_id, muted_by: username,
reason: "Muted from Slack") in error handling: invoke the command inside a
begin/rescue block (or handle the returned result if the command returns a
status), log or notify failures (using Rails.logger or an existing logger)
including the error/exception and error_id, and only update the Slack message to
"Muted" when the mute call succeeds; on failure update Slack with an
error/failure message so the user sees the correct outcome.
In `@Gemfile`:
- Line 11: Add an inline TODO comment next to the gem declaration for
rails_error_dashboard (the gem "rails_error_dashboard", github:
"j4rs/rails_error_dashboard", branch: "feature/mute-errors") that notes to
switch back to the official gem once the upstream PR is merged (include PR link
or issue ID if known and an owner or date to revisit); this will make it easy to
find and revert the GitHub-forked branch when the upstream change lands.
In `@spec/integration/mute_notification_spec.rb`:
- Around line 61-77: The test description "resumes Slack notifications after
unmuting" is misleading because the spec also resolves the error to trigger a
reopened notification; either update the it block description to something like
"resumes Slack notifications after unmuting and resolving (reopened)" or add a
one-line comment above the resolve step (the error_log.update!(resolved: true,
resolved_at: Time.current) call) explaining that resolving is required to
generate a "reopened" notification on the next occurrence; change the string or
add the comment in the it block containing the SlackErrorNotificationJob
expectation to make the intent clear.
In `@spec/requests/slack_interactions_spec.rb`:
- Around line 80-87: The test for "responds with already muted when error is
muted" currently only checks the HTTP status; capture the error_log's
pre-request state (e.g., muted and muted_at), perform the POST to
"/slack/interactions" using build_body(action_id: "mute_error_#{error_log.id}",
error_id: error_log.id) and signed_headers(body), then reload the error_log and
assert that muted remains true and muted_at did not change (or equals the
previously saved timestamp) to ensure no state change; reference the error_log
variable, build_body helper, and the POST to "/slack/interactions" when adding
the assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b9d6756d-0dc3-4066-b81a-e3d1a87a34d6
⛔ Files ignored due to path filters (1)
Gemfile.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
Gemfileapp/controllers/slack_interactions_controller.rbconfig/initializers/rails_error_dashboard.rbspec/integration/mute_notification_spec.rbspec/requests/slack_interactions_spec.rb
Comment on lines
+79
to
+81
| username = payload.dig("user", "username") || "someone" | ||
| RailsErrorDashboard::Commands::MuteError.call(error_id, muted_by: username, reason: "Muted from Slack") | ||
|
|
There was a problem hiding this comment.
Missing error handling for MuteError.call.
If RailsErrorDashboard::Commands::MuteError.call raises an exception or fails silently, the Slack message will still be updated to show "Muted" even though the mute operation may not have succeeded. Consider wrapping this in error handling or verifying the result.
🛡️ Proposed fix
username = payload.dig("user", "username") || "someone"
- RailsErrorDashboard::Commands::MuteError.call(error_id, muted_by: username, reason: "Muted from Slack")
+ begin
+ RailsErrorDashboard::Commands::MuteError.call(error_id, muted_by: username, reason: "Muted from Slack")
+ rescue => e
+ Rails.logger.error("[SlackInteractions] MuteError failed: #{e.message}")
+ respond_to_slack(payload, "Failed to mute error: #{e.message}")
+ return
+ end
user_mention = "@#{username}"📝 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
| username = payload.dig("user", "username") || "someone" | |
| RailsErrorDashboard::Commands::MuteError.call(error_id, muted_by: username, reason: "Muted from Slack") | |
| username = payload.dig("user", "username") || "someone" | |
| begin | |
| RailsErrorDashboard::Commands::MuteError.call(error_id, muted_by: username, reason: "Muted from Slack") | |
| rescue => e | |
| Rails.logger.error("[SlackInteractions] MuteError failed: #{e.message}") | |
| respond_to_slack(payload, "Failed to mute error: #{e.message}") | |
| return | |
| end | |
| user_mention = "@#{username}" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/controllers/slack_interactions_controller.rb` around lines 79 - 81, Wrap
the call to RailsErrorDashboard::Commands::MuteError.call(error_id, muted_by:
username, reason: "Muted from Slack") in error handling: invoke the command
inside a begin/rescue block (or handle the returned result if the command
returns a status), log or notify failures (using Rails.logger or an existing
logger) including the error/exception and error_id, and only update the Slack
message to "Muted" when the mute call succeeds; on failure update Slack with an
error/failure message so the user sees the correct outcome.
Comment on lines
+86
to
+102
| updated_blocks = original_blocks.flat_map do |block| | ||
| if block["type"] == "actions" | ||
| # Keep View Source and Resolve buttons, remove Mute button | ||
| remaining = block["elements"]&.reject { |e| e["action_id"]&.start_with?("mute_error_") } || [] | ||
| [ | ||
| { type: "actions", elements: remaining }, | ||
| { | ||
| type: "context", | ||
| elements: [ | ||
| { type: "mrkdwn", text: ":bell_slash: *Muted* by #{user_mention}" } | ||
| ] | ||
| } | ||
| ] | ||
| else | ||
| [block] | ||
| end | ||
| end |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider extracting shared block manipulation logic.
The flat_map pattern for updating Slack message blocks is duplicated between resolve_error (lines 46-61) and mute_error (lines 86-102). Extracting a helper method would reduce cyclomatic complexity and improve maintainability.
♻️ Proposed refactor
# Add this private method:
def update_action_blocks(original_blocks, filter_action_prefix:, context_text:)
original_blocks.flat_map do |block|
if block["type"] == "actions"
remaining = block["elements"]&.reject { |e| e["action_id"]&.start_with?(filter_action_prefix) } || []
[
{ type: "actions", elements: remaining },
{ type: "context", elements: [{ type: "mrkdwn", text: context_text }] }
]
else
[block]
end
end
end
# Then in mute_error:
updated_blocks = update_action_blocks(
original_blocks,
filter_action_prefix: "mute_error_",
context_text: ":bell_slash: *Muted* by #{user_mention}"
)🧰 Tools
🪛 RuboCop (1.85.1)
[convention] 95-95: Put a comma after the last item of a multiline array.
(Style/TrailingCommaInArrayLiteral)
[convention] 97-97: Put a comma after the last item of a multiline array.
(Style/TrailingCommaInArrayLiteral)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/controllers/slack_interactions_controller.rb` around lines 86 - 102, The
duplicated flat_map block-manipulation in mute_error and resolve_error should be
extracted into a private helper (e.g., update_action_blocks) to reduce
duplication and cyclomatic complexity; implement
update_action_blocks(original_blocks, filter_action_prefix:, context_text:) that
performs the flat_map, rejects elements by action_id prefix and appends a
context block, then replace the inline logic in both mute_error and
resolve_error to call this helper passing "mute_error_" / "resolve_error_" (or
appropriate prefixes) and the respective context_text (e.g., ":bell_slash:
*Muted* by #{user_mention}").
| # Use the Puma web server [https://github.com/puma/puma] | ||
| gem "puma", ">= 5.0" | ||
| gem "rails_error_dashboard" | ||
| gem "rails_error_dashboard", github: "j4rs/rails_error_dashboard", branch: "feature/mute-errors" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add a TODO comment to track switching back to the official gem.
Using a GitHub fork branch is appropriate for development, but there's no inline reminder to switch back once the upstream PR is merged. This could be forgotten and lead to running a divergent fork in production long-term.
📝 Proposed fix
-gem "rails_error_dashboard", github: "j4rs/rails_error_dashboard", branch: "feature/mute-errors"
+# TODO: Switch back to official gem once https://github.com/AnjanJ/rails_error_dashboard/pull/92 is merged
+gem "rails_error_dashboard", github: "j4rs/rails_error_dashboard", branch: "feature/mute-errors"📝 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
| gem "rails_error_dashboard", github: "j4rs/rails_error_dashboard", branch: "feature/mute-errors" | |
| # TODO: Switch back to official gem once https://github.com/AnjanJ/rails_error_dashboard/pull/92 is merged | |
| gem "rails_error_dashboard", github: "j4rs/rails_error_dashboard", branch: "feature/mute-errors" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Gemfile` at line 11, Add an inline TODO comment next to the gem declaration
for rails_error_dashboard (the gem "rails_error_dashboard", github:
"j4rs/rails_error_dashboard", branch: "feature/mute-errors") that notes to
switch back to the official gem once the upstream PR is merged (include PR link
or issue ID if known and an owner or date to revisit); this will make it easy to
find and revert the GitHub-forked branch when the upstream change lands.
Comment on lines
+61
to
+77
| it "resumes Slack notifications after unmuting" do | ||
| # Create and mute the error | ||
| post "/api/v1/errors", params: error_payload.to_json, headers: headers | ||
| error_log = RailsErrorDashboard::ErrorLog.last | ||
| error_log.update!(muted: true, muted_at: Time.current) | ||
|
|
||
| # Unmute it | ||
| error_log.update!(muted: false, muted_at: nil) | ||
|
|
||
| # Resolve it so the next occurrence triggers a "reopened" notification | ||
| error_log.update!(resolved: true, resolved_at: Time.current) | ||
|
|
||
| expect(RailsErrorDashboard::SlackErrorNotificationJob).to receive(:perform_later).at_least(:once) | ||
|
|
||
| post "/api/v1/errors", params: error_payload.to_json, headers: headers | ||
| expect(response).to have_http_status(:created) | ||
| end |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Test name doesn't fully reflect the test scenario.
The test involves both unmuting and resolving the error (lines 68-71), but the test name only mentions "unmuting". The resolve step is necessary to trigger a notification (reopened scenario), but this could confuse future readers.
📝 Proposed improvement
- it "resumes Slack notifications after unmuting" do
+ it "resumes Slack notifications after unmuting and reopening a resolved error" doOr add a comment explaining why the resolve step is necessary:
# Unmute it
error_log.update!(muted: false, muted_at: nil)
- # Resolve it so the next occurrence triggers a "reopened" notification
+ # Mark as resolved so the next occurrence triggers a notification
+ # (deduplicated errors only notify on first occurrence or when reopened)
error_log.update!(resolved: true, resolved_at: Time.current)📝 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
| it "resumes Slack notifications after unmuting" do | |
| # Create and mute the error | |
| post "/api/v1/errors", params: error_payload.to_json, headers: headers | |
| error_log = RailsErrorDashboard::ErrorLog.last | |
| error_log.update!(muted: true, muted_at: Time.current) | |
| # Unmute it | |
| error_log.update!(muted: false, muted_at: nil) | |
| # Resolve it so the next occurrence triggers a "reopened" notification | |
| error_log.update!(resolved: true, resolved_at: Time.current) | |
| expect(RailsErrorDashboard::SlackErrorNotificationJob).to receive(:perform_later).at_least(:once) | |
| post "/api/v1/errors", params: error_payload.to_json, headers: headers | |
| expect(response).to have_http_status(:created) | |
| end | |
| it "resumes Slack notifications after unmuting and reopening a resolved error" do | |
| # Create and mute the error | |
| post "/api/v1/errors", params: error_payload.to_json, headers: headers | |
| error_log = RailsErrorDashboard::ErrorLog.last | |
| error_log.update!(muted: true, muted_at: Time.current) | |
| # Unmute it | |
| error_log.update!(muted: false, muted_at: nil) | |
| # Resolve it so the next occurrence triggers a "reopened" notification | |
| error_log.update!(resolved: true, resolved_at: Time.current) | |
| expect(RailsErrorDashboard::SlackErrorNotificationJob).to receive(:perform_later).at_least(:once) | |
| post "/api/v1/errors", params: error_payload.to_json, headers: headers | |
| expect(response).to have_http_status(:created) | |
| end |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@spec/integration/mute_notification_spec.rb` around lines 61 - 77, The test
description "resumes Slack notifications after unmuting" is misleading because
the spec also resolves the error to trigger a reopened notification; either
update the it block description to something like "resumes Slack notifications
after unmuting and resolving (reopened)" or add a one-line comment above the
resolve step (the error_log.update!(resolved: true, resolved_at: Time.current)
call) explaining that resolving is required to generate a "reopened"
notification on the next occurrence; change the string or add the comment in the
it block containing the SlackErrorNotificationJob expectation to make the intent
clear.
Comment on lines
+80
to
+87
| it "responds with already muted when error is muted" do | ||
| error_log.update!(muted: true, muted_at: Time.current) | ||
| body = build_body(action_id: "mute_error_#{error_log.id}", error_id: error_log.id) | ||
|
|
||
| post "/slack/interactions", params: body, headers: signed_headers(body) | ||
|
|
||
| expect(response).to have_http_status(:ok) | ||
| end |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Strengthen the "already muted" test by verifying no state change.
The test confirms the response status but doesn't verify that the error record wasn't modified. Adding an assertion would ensure the idempotent behavior is properly tested.
💚 Proposed improvement
it "responds with already muted when error is muted" do
error_log.update!(muted: true, muted_at: Time.current)
+ original_muted_at = error_log.muted_at
body = build_body(action_id: "mute_error_#{error_log.id}", error_id: error_log.id)
post "/slack/interactions", params: body, headers: signed_headers(body)
expect(response).to have_http_status(:ok)
+ expect(error_log.reload.muted_at).to eq(original_muted_at)
end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@spec/requests/slack_interactions_spec.rb` around lines 80 - 87, The test for
"responds with already muted when error is muted" currently only checks the HTTP
status; capture the error_log's pre-request state (e.g., muted and muted_at),
perform the POST to "/slack/interactions" using build_body(action_id:
"mute_error_#{error_log.id}", error_id: error_log.id) and signed_headers(body),
then reload the error_log and assert that muted remains true and muted_at did
not change (or equals the previously saved timestamp) to ensure no state change;
reference the error_log variable, build_body helper, and the POST to
"/slack/interactions" when adding the assertion.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
j4rs/rails_error_dashboardbranchfeature/mute-errors)SlackErrorNotificationJobDepends on
Test plan
spec/requests/slack_interactions_spec.rb— 4 examples, 0 failures (mute action, already muted, not found, signature rejection)spec/integration/mute_notification_spec.rb— 3 examples, 0 failures (unmuted sends Slack, muted skips Slack, unmute resumes Slack)🤖 Generated with Claude Code
Summary by CodeRabbit