Conversation
Walkthroughこの変更は、カスタムイベント通知システム「Newspaper」からRails標準の「ActiveSupport::Notifications」への移行を行っています。プロダクト更新およびコメント作成のイベント発行を文字列イベント名のinstrument呼び出しへ置換し、購読者のコールシグネチャをActiveSupport::Notificationsの5引数形式に合わせ、購読登録をinitializersへ移動しています。 Changes
Sequence Diagram(s)sequenceDiagram
participant Controller as ProductsController
participant ASN as ActiveSupport::Notifications
participant Watcher as ProductUpdateNotifierForWatcher
participant Checker as ProductUpdateNotifierForChecker
Controller->>ASN: instrument('product.update', product:..., current_user:...)
ASN->>Watcher: call(_name, _started, _finished, _id, payload)
ASN->>Checker: call(_name, _started, _finished, _id, payload)
sequenceDiagram
participant Callback as Comment::AfterCreateCallback
participant ASN as ActiveSupport::Notifications
participant Notifier as CommentNotifier
Callback->>ASN: instrument('came.comment', comment: comment)
ASN->>Notifier: call(_name, _started, _finished, _id, payload)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15–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/products_controller.rbrubocop-minitest extension supports plugin, specify app/models/comment/after_create_callback.rbrubocop-minitest extension supports plugin, specify app/models/comment_notifier.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. ✨ 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
|
matuaya
force-pushed
the
chore/replace-newspaper-with-activesupport-nofitications-for-product-and-comment
branch
from
ea4323e to
d211703
Compare
matuaya
force-pushed
the
chore/replace-newspaper-with-activesupport-nofitications-for-product-and-comment
branch
3 times, most recently
from
a8b05d9 to
6373216
Compare
matuaya
force-pushed
the
chore/replace-newspaper-with-activesupport-nofitications-for-product-and-comment
branch
2 times, most recently
from
98a7976 to
0523d50
Compare
|
@jun-kondo |
|
@matuaya |
|
@jun-kondo |
matuaya
changed the title
|
@jun-kondo @komagata |
|
📝 リリース直前なので、このリリースが終わり次第mainにマージしたいと思います。 |
|
@matuaya リリースにあたってmainとproductionが変わった結果conflictが増えてしまっています、すみません。 |
|
📝 下記でmainの最新が取り込めると思います。 |
matuaya
force-pushed
the
chore/replace-newspaper-with-activesupport-nofitications-for-product-and-comment
branch
from
0523d50 to
12075b4
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
app/models/comment_notifier.rb (3)
12-12: 文言はI18nに退避して再利用・翻訳容易性を確保ハードコードされた日本語メッセージをI18n化すると他画面・通知との一貫性が高まります。
提案差分:
- message: "相談部屋で#{comment.sender.login_name}さんからコメントがありました。", + message: I18n.t('comment_notifier.came_comment.message', sender: comment.sender.login_name),合わせて、例えば以下を
config/locales/comment_notifier.ja.ymlに追加してください:ja: comment_notifier: came_comment: message: "相談部屋で%{sender}さんからコメントがありました。"
5-7: 異常系の可観測性を少し上げる（任意）
payload[:comment]が欠落した場合にログを1行出すと原因追跡が容易になります。提案差分:
- return if comment.nil? + if comment.nil? + Rails.logger.warn('[CommentNotifier] payload without :comment; skip') + return + end
8-14: アンカーはルーティングヘルパのanchor:オプションで付与して安全に結合しましょう既存の
config/environments/*では全ての環境（development, test, production）で
config.action_mailer.default_url_optionsが設定されているため、
メール内で絶対 URL を組み立てる場合はpolymorphic_urlの利用が可能です。• 修正対象
- ファイル：
app/models/comment_notifier.rb- 対象行：リンク生成部（8–14行目付近）
• 提案差分
- commentable_path = Rails.application.routes.url_helpers.polymorphic_path(comment.commentable) - ActivityDelivery.with( - comment:, - receiver: comment.receiver, - message: "相談部屋で#{comment.sender.login_name}さんからコメントがありました。", - link: "#{commentable_path}#latest-comment" - ).notify(:came_comment) + # アンカーはルーティングヘルパのanchorオプションで付与する + ActivityDelivery.with( + comment:, + receiver: comment.receiver, + message: "相談部屋で#{comment.sender.login_name}さんからコメントがありました。", + link: Rails.application.routes.url_helpers.polymorphic_url( + comment.commentable, + anchor: 'latest-comment' + ) + ).notify(:came_comment)• 補足
- 絶対 URL が不要であれば
polymorphic_path(..., anchor: 'latest-comment')を利用してください- メール本文で絶対 URL が必要な場合は
polymorphic_urlのまま、もしくはdefault_url_options[:host]の設定を再度ご確認くださいapp/models/product_update_notifier_for_watcher.rb (3)
7-7: nilチェックの簡素化と可読性向上（安全ナビゲーション演算子）
!current_user.nil? && current_user.admin_or_mentor?はcurrent_user&.admin_or_mentor?に置換可能です。演算子優先順位の読み違いリスクも下がります。なお、product.watches.nil?は関連が常にCollectionProxyを返す想定なら常にfalseになりうるため、将来的には削除検討もありです。- return if product.wip? || product.watches.nil? || !current_user.nil? && current_user.admin_or_mentor? + return if product.wip? || product.watches.nil? || current_user&.admin_or_mentor?
9-11: 重複除去と早期returnで無駄な処理を回避
- 二重watchレコードが存在した場合の重複通知を避けるため、SQLレベルで
distinctを付与。- 宛先が空のときは
Userの問い合わせや配送処理に進まず早期return。- receiver_ids = product.watches.where.not(user_id: product.user_id).pluck(:user_id) + receiver_ids = product.watches.where.not(user_id: product.user_id).distinct.pluck(:user_id) + return if receiver_ids.empty?
20-22: 大量宛先時のメモリフットプリント削減（find_each）
receiversはRelationなのでfind_eachによりバッチ処理できます。大量ユーザーでも一括ロードを避けられます。- receivers.each do |receiver| + receivers.find_each do |receiver| ActivityDelivery.with(product:, receiver:).notify(:product_update) end
📜 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 (5)
app/controllers/products_controller.rb(1 hunks)app/models/comment/after_create_callback.rb(1 hunks)app/models/comment_notifier.rb(1 hunks)app/models/product_update_notifier_for_checker.rb(1 hunks)app/models/product_update_notifier_for_watcher.rb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- app/models/product_update_notifier_for_checker.rb
- app/models/comment/after_create_callback.rb
- app/controllers/products_controller.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:
app/models/product_update_notifier_for_watcher.rbapp/models/comment_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 (5)
app/models/comment_notifier.rb (2)
4-4: ActiveSupport::Notificationsの5引数シグネチャ対応は問題なしブロック引数を使わないことを明示するための先頭アンダースコアも適切です。移行としてはLGTMです。
4-4: initializerでのsubscribeはインスタンス渡しになっており問題ありません
config/initializers/active_support_notifications.rbの32行目で以下のように書かれており、インスタンスを直接渡す形式になっています。
- ActiveSupport::Notifications.subscribe('came.comment', CommentNotifier.new)
よって、
CommentNotifier#callは意図どおりインスタンスに対して呼び出され、動作上の問題はありません。app/models/product_update_notifier_for_watcher.rb (3)
4-4: AS::Notificationsの5引数コールシグネチャ対応は妥当です未使用引数を先頭アンダースコアで明示しており読みやすいです。PR目的にも合致しています。
11-15: トランザクション境界の確認（送信タイミングの安定化）通知はDBコミット後（
after_*_commit）に発火していると失敗時のダングリング通知を避けられます。instrument発火位置がコミット後かを確認してください。上の確認スクリプトの「after_commit確認」節で、
ActiveSupport::Notifications.instrument("product.update", ...)がafter_update_commitやafter_commitの中から呼ばれているかを確認できます。必要なら発火箇所を*_commit系コールバックへ移動しましょう。
4-5: subscribe側のイベント名とpayloadキーは整合しています
- config/initializers/active_support_notifications.rb:30 で
ActiveSupport::Notifications.subscribe('product.update', ProductUpdateNotifierForWatcher.new)が定義されており- app/controllers/products_controller.rb:68 で
ActiveSupport::Notifications.instrument('product.update', { product: @product, current_user: })として発行されているためイベント名とpayloadキー（:product, :current_user）が一致していることを確認しました。対応不要です。
|
@komagata |
komagata
deleted the
chore/replace-newspaper-with-activesupport-nofitications-for-product-and-comment
branch
|
@matuaya こちら本番で通知が来ているのを確認してますー👍無事に通知が飛んでました✨ |
4 participants
Issue
概要
ProductUpdateNotifierForWatcher、ProductUpdateNotifierForChecker、CommentNotifierをNewspaperからActiveSupport::Notificationsのsubscribeの仕組みに移行しました。
AnswererWatcherをnewspaperからActiveSupport::Notificationsに移行のPRを参考にしました。
変更確認方法
ブランチ
chore/replace-newspaper-with-activesupport-nofitications-for-product-and-commentをローカルに取り込むUIでの確認
＊メールは http://localhost:3000/letter_opener/ で確認
Screenshot
UIに変化がなかったためスクリーンショットは省略しています。
Summary by CodeRabbit
新機能
リファクタ
テスト