QuestionNotifierをnewspaperからActiveSupport::Notificationsに移行#9093
QuestionNotifierをnewspaperからActiveSupport::Notificationsに移行#9093
Conversation
WalkthroughQuestionsController の通知発行を ActiveSupport::Notifications.instrument に統一し、QuestionNotifier#call を ActiveSupport::Notifications の 5 引数シグネチャに変更、active_support_notifications 初期化子で購読を登録し、newspaper 初期化子から該当購読を削除した。 Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as ユーザ
participant QC as QuestionsController
participant AS as ActiveSupport::Notifications
participant QN as QuestionNotifier
rect rgb(240,248,255)
Note over U,QC: 質問作成フロー
U->>QC: POST /questions
QC->>QC: @question.save
alt 保存成功
QC->>AS: instrument('question.create', question: @question)
AS-->>QN: call(_name,_started,_finished,_id, payload)
QC-->>U: redirect
else 保存失敗
QC-->>U: render :new
end
end
rect rgb(245,245,240)
Note over U,QC: 更新（wip 変更時）
U->>QC: PATCH /questions/:id
QC->>QC: @question.update(...)
alt saved_change_to_wip? == true
QC->>AS: instrument('question.update', question: @question)
AS-->>QN: call(_name,_started,_finished,_id, payload)
QC-->>U: redirect
else
QC-->>U: redirect
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 RuboCop (1.76.1)app/controllers/questions_controller.rbrubocop-minitest extension supports plugin, specify Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
app/controllers/questions_controller.rb(2 hunks)app/models/question_notifier.rb(1 hunks)config/initializers/active_support_notifications.rb(1 hunks)config/initializers/newspaper.rb(0 hunks)
💤 Files with no reviewable changes (1)
- config/initializers/newspaper.rb
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rb
⚙️ CodeRabbit configuration file
**/*.rb: # refactoring
- まずFat Controllerを避け、次にFat Modelを避ける。
- Serviceクラスの乱用を避ける。
- controller concernを作ろうとしたらPORO(Plain Old Ruby Object)やActiveRecordモデルでの実装で代替できないか検討する。
Rails Patterns
- ViewHelperにメソッドを実装する時にはまずDecoratorパターンを使うことを検討する。（active_decorator gemを導入しているのでそれを使う）
- 複雑なActiveRecordクエリがあり、再利用できそうな場合はQueryObjectパターンを検討する。（rails-patterns gemを導入しているのでそれのQuery機能を使う）
- Viewにpartialを作る場合はViewComponentを使うことを検討する。
- 複数のActiveRecordモデルを操作する１つの責務がある時や外部APIとやりとりする処理がある場合にはInteractorオブジェクトパターンを検討する。（interactor gemを導入しているのでそれを使う）
- 複数のInteractorを実行するような処理がある場合Organizerオブジェクトパターンを検討する。（interactor gemを導入しており、その中にOrganizerの機能があるので使う）
Files:
config/initializers/active_support_notifications.rbapp/controllers/questions_controller.rbapp/models/question_notifier.rb
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build_and_test
🔇 Additional comments (3)
app/models/question_notifier.rb (1)
4-4: ActiveSupport::Notificationsの標準インターフェースに準拠メソッドシグネチャがActiveSupport::Notificationsの購読者インターフェースに正しく更新されています。未使用のパラメータに適切にアンダースコアのプレフィックスが付けられており、payloadの使用方法は従来通り維持されています。
config/initializers/active_support_notifications.rb (1)
44-46: QuestionNotifierの購読設定が適切に追加されているQuestionNotifierがActiveSupport::Notificationsの仕組みに正しく統合されており、question.createとquestion.updateイベントに適切に購読されています。他のNotifierクラスと同様のパターンに従っているため、一貫性があります。
app/controllers/questions_controller.rb (1)
60-60: 質問作成時のActiveSupport::Notifications計測を追加質問作成時にActiveSupport::Notifications.instrumentが正しく呼び出されており、payloadにquestionオブジェクトが含まれています。Newspaperの既存の通知と並行して実行されるため、移行段階として適切です。
smallmonkeykey
force-pushed
the
chore/replace-question_notifier_v2
branch
from
1ec08df to
1917701
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
config/initializers/active_support_notifications.rb (1)
54-56: 開発環境でのコードリロード時の“多重購読”を避けるための軽微なガードを追加しませんか？（任意）本ファイル全体が to_prepare 内で subscribe しているため、開発環境の再読み込みを繰り返すと同一イベントに同一購読者が積み上がる懸念があります（既存実装も同様）。本PR範囲では最低限 QuestionNotifier だけでもトークン管理で unsubscribe→再購読するのが安全です。全体対応は別PRでもOKです。
以下の diff を該当行に適用してください（トークン管理で重複を防止）。
- question_notifier = QuestionNotifier.new - ActiveSupport::Notifications.subscribe('question.create', question_notifier) - ActiveSupport::Notifications.subscribe('question.update', question_notifier) + # 重複購読防止（開発時のコードリロード対策） + QUESTION_NOTIFIER_TOKENS&.each { |t| ActiveSupport::Notifications.unsubscribe(t) } + QUESTION_NOTIFIER_TOKENS = [] unless defined?(QUESTION_NOTIFIER_TOKENS) + + question_notifier = QuestionNotifier.new + QUESTION_NOTIFIER_TOKENS << ActiveSupport::Notifications.subscribe('question.create', question_notifier) + QUESTION_NOTIFIER_TOKENS << ActiveSupport::Notifications.subscribe('question.update', question_notifier)補足（この定義は to_prepare ブロックの外側・同ファイル先頭付近に追加してください）:
# 重複定義防止 QUESTION_NOTIFIER_TOKENS = [] unless defined?(QUESTION_NOTIFIER_TOKENS)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
app/controllers/questions_controller.rb(2 hunks)app/models/question_notifier.rb(1 hunks)config/initializers/active_support_notifications.rb(1 hunks)config/initializers/newspaper.rb(0 hunks)
💤 Files with no reviewable changes (1)
- config/initializers/newspaper.rb
🚧 Files skipped from review as they are similar to previous changes (2)
- app/controllers/questions_controller.rb
- app/models/question_notifier.rb
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rb
⚙️ CodeRabbit configuration file
**/*.rb: # refactoring
- まずFat Controllerを避け、次にFat Modelを避ける。
- Serviceクラスの乱用を避ける。
- controller concernを作ろうとしたらPORO(Plain Old Ruby Object)やActiveRecordモデルでの実装で代替できないか検討する。
Rails Patterns
- ViewHelperにメソッドを実装する時にはまずDecoratorパターンを使うことを検討する。（active_decorator gemを導入しているのでそれを使う）
- 複雑なActiveRecordクエリがあり、再利用できそうな場合はQueryObjectパターンを検討する。（rails-patterns gemを導入しているのでそれのQuery機能を使う）
- Viewにpartialを作る場合はViewComponentを使うことを検討する。
- 複数のActiveRecordモデルを操作する１つの責務がある時や外部APIとやりとりする処理がある場合にはInteractorオブジェクトパターンを検討する。（interactor gemを導入しているのでそれを使う）
- 複数のInteractorを実行するような処理がある場合Organizerオブジェクトパターンを検討する。（interactor gemを導入しており、その中にOrganizerの機能があるので使う）
Files:
config/initializers/active_support_notifications.rb
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build_and_test
🔇 Additional comments (1)
config/initializers/active_support_notifications.rb (1)
54-56: AS::Notifications への置き換えは妥当。イベント名・引数シグネチャとも整合しています。QuestionNotifier をインスタンスで購読させる実装は ActiveSupport::Notifications の期待（call(name, started, finished, id, payload)）に合致。question.create / question.update というイベント名も、Controllers 側の instrument 呼び出しと一致している前提で問題ありません。
| question_notifier = QuestionNotifier.new | ||
| ActiveSupport::Notifications.subscribe('question.create', question_notifier) | ||
| ActiveSupport::Notifications.subscribe('question.update', question_notifier) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Newspaper 側の購読が残っていないか／発火条件が想定どおりかを確認してください。
二重通知や不要通知（単なる編集時の update）を避けるために以下の観点を念のため検証をお願いします。
- Newspaper.publish（または newspaper 初期化子での question_* イベント購読）が完全に削除されていること
- QuestionsController で instrument が「WIP → 公開」遷移時のみ発火していること（単なる編集では発火しないこと）
- QuestionNotifier#call が 5 引数シグネチャになっていること
以下のスクリプトで機械的に確認できます（リポジトリルートで実行）。
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Newspaper 残骸チェック"
rg -nP "Newspaper\.(publish|subscribe)|question_(create|update)" -S -C2 || echo "OK: Newspaper 関連は見つかりませんでした"
echo
echo "2) AS::Notifications instrument の確認（QuestionsController）"
rg -nP "ActiveSupport::Notifications\.instrument\(['\"]question\.(create|update)['\"]" app/controllers -C3
echo
echo "3) QuestionNotifier#call の 5 引数確認"
ast-grep --pattern $'class QuestionNotifier {\n $$$\n def call($_, $_, $_, $_, $_)\n $$$\n end\n $$$\n}' --type ruby || {
echo "Fallback: ripgrep で def call を表示"; rg -n "class QuestionNotifier|def call\(" -C2 app -g '**/*.rb'
}
echo
echo "4) update 発火条件の簡易確認（WIP->公開周辺の条件分岐の有無）"
rg -nP "(wip|published|state).*update" app/controllers/questions_controller.rb -C3 || echo "注意: コントローラ内の条件分岐が見当たりません。Notifier 側で判定している可能性があります。"Length of output: 25617
Newspaper.publish の削除が必要です
以下の箇所でまだ Newspaper.publish による question イベントの発行が残っており、config/initializers/newspaper.rb 側では購読先が定義されていないため、不要な冗長コードとなっています。削除をお願いします。
- app/controllers/questions_controller.rb
- 行 61:
Newspaper.publish(:question_create, { question: @question }) - 行 73:
Newspaper.publish(:question_update, { question: @question })
- 行 61:
他のチェック結果:
- ActiveSupport::Notifications.instrument は
saved_change_to_wip?でのみ発火しており、「WIP→公開」の遷移でのみ通知が発生する仕様になっています（問題ありません）。 - QuestionNotifier#call は 5 引数シグネチャ（
_name, _started, _finished, _unique_id, payload）となっており、期待どおりです。
🤖 Prompt for AI Agents
In config/initializers/active_support_notifications.rb around lines 54 to 56:
remove the remaining Newspaper.publish calls in
app/controllers/questions_controller.rb (specifically at lines 61 and 73:
Newspaper.publish(:question_create, { question: @question }) and
Newspaper.publish(:question_update, { question: @question })) because those
events are not subscribed in config/initializers/newspaper.rb; delete those two
calls so only ActiveSupport::Notifications is used for question events and run
tests to ensure no other code references Newspaper.publish for questions.
There was a problem hiding this comment.
@smallmonkeykey ありがとうございます！Newspaper.publishの呼び出しを削除していただき、完全にActiveSupport::Notificationsへの移行が完了しましたね。これで二重通知の心配もなくなり、コードもより一貫性のある実装になりました。
/| /|
( ^.^ )
o_(")(")
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
NewspaperとNotificationsの併用に関する懸念は解消済みのため
|
@komagata |
2 participants
連絡
このプルリクエストは、過去のプルリクエスト#8982の履歴を整理して再度作成したものです。
前回は、rebase中のコンフリクト処理に問題があり、履歴が複雑になったため、cherry-pickで必要な変更のみを抽出して作り直しています。
QuestionNotifierをnewspaperからActiveSupport::Notificationsに移行しました。
Issue
概要
log/development.logへMessage to Discord.が出力される仕組みになってます。変更確認方法
chore/replace-question_notifierブランチをローカルに取り込むforeman start -f Procfile.devでローカルで立ち上げますtail -f log/development.log | grep "Message to Discord"通知が飛んだ場合、Message to Discord. という出力があるかを確認できます
hajimeまたはkimuraでログインを行いますhttp://localhost:3000/questions/newで質問を作成します以下の3パターンを検証してください
1：WIP保存のみ
質問をWIPで保存してください
期待結果：Message to Discord. のログが出力されないことを確認してください
2：WIP → 公開に変更
上記で作成したWIPの質問を
質問を公開ボタンをクリックし公開してください期待結果：ログに Message to Discord. が出力されることを確認してください
3：質問を作成し、
登録するをクリックしてください期待結果：ログに Message to Discord. が出力されることを確認してください
例
このような表示がされます。
Screenshot
見た目に変更はないためスクリーンショットはありません。
Summary by CodeRabbit