ChecksController内のnewspaperをActiveSupport::Notificationsに移行#9247
ChecksController内のnewspaperをActiveSupport::Notificationsに移行#9247
Conversation
Walkthroughチェックの作成・削除処理をトランザクション内で save!/destroy! に置き換え、イベント発行を 変更内容
シーケンス図sequenceDiagram
actor User
participant Controller
participant DB
participant Notifications as "ActiveSupport::Notifications"
participant Callbacks
User->>Controller: POST /checks または DELETE /checks/:id
Controller->>DB: begin transaction
alt 成功
DB->>DB: save! / destroy!
Controller->>Notifications: instrument('check.create'/'check.cancel')
DB-->>Controller: commit
Notifications-->>Callbacks: after_*_commit を発火
Callbacks->>DB: コールバック内副作用（例: 学習状態変更）
Controller->>User: 201 / 204 (またはリダイレクト)
else エラー
DB-->>DB: rollback
Controller->>Controller: rescue StandardError, log
Controller->>User: エラー応答 (JSON またはリダイレクト with alert)
end
推定コードレビュー工数🎯 3 (中程度) | ⏱️ ~20 minutes
Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used📓 Path-based instructions (1)**/*.rb⚙️ CodeRabbit configuration file
Files:
🧠 Learnings (25)📓 Common learnings📚 Learning: 2025-10-22T06:04:36.036ZApplied to files:
📚 Learning: 2025-09-04T01:27:23.422ZApplied to files:
📚 Learning: 2025-09-04T01:27:23.422ZApplied to files:
📚 Learning: 2025-10-22T12:22:33.168ZApplied to files:
📚 Learning: 2025-09-01T22:31:57.345ZApplied to files:
📚 Learning: 2025-09-01T22:31:57.345ZApplied to files:
📚 Learning: 2025-09-12T21:18:00.834ZApplied to files:
📚 Learning: 2025-07-23T21:11:21.826ZApplied to files:
📚 Learning: 2025-09-12T21:16:47.639ZApplied to files:
📚 Learning: 2025-09-15T07:39:39.350ZApplied to files:
📚 Learning: 2025-09-12T21:17:45.295ZApplied to files:
📚 Learning: 2025-09-12T21:17:26.029ZApplied to files:
📚 Learning: 2025-09-04T01:39:22.261ZApplied to files:
📚 Learning: 2025-07-23T21:11:21.826ZApplied to files:
📚 Learning: 2025-07-23T20:31:13.856ZApplied to files:
📚 Learning: 2025-07-15T12:58:02.735ZApplied to files:
📚 Learning: 2025-08-24T07:39:34.701ZApplied to files:
📚 Learning: 2025-09-04T01:50:47.773ZApplied to files:
📚 Learning: 2025-06-29T03:44:15.179ZApplied to files:
📚 Learning: 2025-07-26T15:24:14.799ZApplied to files:
📚 Learning: 2025-09-02T19:01:08.804ZApplied to files:
📚 Learning: 2025-07-23T20:42:19.974ZApplied to files:
📚 Learning: 2025-08-01T12:57:10.349ZApplied to files:
📚 Learning: 2025-11-03T04:53:33.395ZApplied to files:
🧬 Code graph analysis (1)app/models/product.rb (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)
🔇 Additional comments (3)
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.81.7)app/controllers/api/checks_controller.rbrubocop-minitest extension supports plugin, specify ... [truncated 255 characters] ... fig_loader_resolver.rb:76:in app/models/product.rbrubocop-minitest extension supports plugin, specify ... [truncated 255 characters] ... fig_loader_resolver.rb:76:in app/controllers/checks_controller.rbrubocop-minitest extension supports plugin, specify ... [truncated 255 characters] ... fig_loader_resolver.rb:76:in
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. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/controllers/checks_controller.rb(2 hunks)
🧰 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/controllers/checks_controller.rb
🧠 Learnings (1)
📓 Common learnings
Learnt from: matuaya
PR: fjordllc/bootcamp#9108
File: app/models/after_user_retirement.rb:0-0
Timestamp: 2025-09-01T22:31:57.345Z
Learning: fjordllc/bootcampプロジェクトにおいて、`Newspaper.publish(:retirement_create, { user: user })`は通知処理ではなく、UnfinishedDataDestroyerやTimesChannelDestroyer等による重要なデータクリーンアップ処理を実行するため、管理者による退会（triggered_by: 'admin'）の場合でも必ず実行する必要がある。
Learnt from: matuaya
PR: fjordllc/bootcamp#9108
File: app/models/after_user_retirement.rb:0-0
Timestamp: 2025-09-01T22:31:57.345Z
Learning: fjordllc/bootcampプロジェクトにおいて、`Newspaper.publish(:retirement_create, { user: user })`はUnfinishedDataDestroyer（未チェックProduct・WIP Report削除、career_pathリセット）とTimesChannelDestroyer（Discord分報チャンネル削除）による重要なデータクリーンアップ処理を実行するため、退会のトリガーが何であろうと（admin、hibernation、user）必ず実行する必要がある。
⏰ 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)
app/controllers/checks_controller.rb (1)
18-18: 通知タイミングが適切です。
@check.saveの成功後にのみイベント通知を送信しており、適切な実装です。イベント名も Rails の命名規則に従っています。
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
app/controllers/checks_controller.rb (1)
50-54: ログメッセージのイベント名を動的にしてください。Line 53のログメッセージが
'check.create'とハードコードされていますが、eventパラメータを使用すべきです。現在の実装では、check.cancelイベントでエラーが発生した場合も'check.create'と表示されてしまいます。以下のdiffを適用してください：
def safe_instrument(event, check) ActiveSupport::Notifications.instrument(event, check) rescue StandardError => e - Rails.logger.warn "[Notifications] 'check.create'サブスクライブ処理でエラーが発生しました。：#{e.message}" + Rails.logger.warn "[Notifications] '#{event}'サブスクライブ処理でエラーが発生しました。：#{e.message}" end
🧹 Nitpick comments (1)
app/models/check.rb (1)
16-33: キャッシュクリアロジックの実装が適切です。以下の点で良い実装になっています：
clear_cacheメソッドで両方のキャッシュクリア処理を呼び出し、統一的なインターフェースを提供delete_report_cacheとdelete_product_cacheでcheckable_typeに応じた適切な処理を実行- ガード句により不要な処理を早期にスキップ
より厳密なカプセル化を行う場合、
clear_cache、delete_report_cache、delete_product_cacheをprivateにすることを検討してください（現在はコールバックから呼ばれるためpublicで問題ありませんが、外部から直接呼ばれることを想定していない場合）。alias sender user validates :user_id, uniqueness: { scope: %i[checkable_id checkable_type] } def receiver checkable.respond_to?(:user) ? checkable.user : nil end + + private def clear_cache delete_report_cache
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/controllers/checks_controller.rb(3 hunks)app/models/check.rb(1 hunks)app/models/check_callbacks.rb(0 hunks)app/models/check_notifier_for_submitter.rb(1 hunks)app/models/product.rb(1 hunks)config/initializers/active_support_notifications.rb(1 hunks)
💤 Files with no reviewable changes (1)
- app/models/check_callbacks.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/check_notifier_for_submitter.rbapp/controllers/checks_controller.rbconfig/initializers/active_support_notifications.rbapp/models/check.rbapp/models/product.rb
🧠 Learnings (2)
📓 Common learnings
Learnt from: sjabcdefin
PR: fjordllc/bootcamp#9247
File: app/controllers/checks_controller.rb:18-18
Timestamp: 2025-10-12T03:08:58.326Z
Learning: ChecksController#createでは、check.saveが成功した後の副作用（ステータス更新、アプリ内通知）で例外が発生しても、checkオブジェクトはロールバックせず、ログ出力のみで対応する方針。
Learnt from: matuaya
PR: fjordllc/bootcamp#9108
File: app/models/after_user_retirement.rb:0-0
Timestamp: 2025-09-01T22:31:57.345Z
Learning: fjordllc/bootcampプロジェクトにおいて、`Newspaper.publish(:retirement_create, { user: user })`は通知処理ではなく、UnfinishedDataDestroyerやTimesChannelDestroyer等による重要なデータクリーンアップ処理を実行するため、管理者による退会（triggered_by: 'admin'）の場合でも必ず実行する必要がある。
Learnt from: matuaya
PR: fjordllc/bootcamp#9108
File: app/models/after_user_retirement.rb:0-0
Timestamp: 2025-09-01T22:31:57.345Z
Learning: fjordllc/bootcampプロジェクトにおいて、`Newspaper.publish(:retirement_create, { user: user })`はUnfinishedDataDestroyer（未チェックProduct・WIP Report削除、career_pathリセット）とTimesChannelDestroyer（Discord分報チャンネル削除）による重要なデータクリーンアップ処理を実行するため、退会のトリガーが何であろうと（admin、hibernation、user）必ず実行する必要がある。
📚 Learning: 2025-10-12T03:08:58.326Z
Learnt from: sjabcdefin
PR: fjordllc/bootcamp#9247
File: app/controllers/checks_controller.rb:18-18
Timestamp: 2025-10-12T03:08:58.326Z
Learning: ChecksController#createでは、check.saveが成功した後の副作用（ステータス更新、アプリ内通知）で例外が発生しても、checkオブジェクトはロールバックせず、ログ出力のみで対応する方針。
Applied to files:
app/controllers/checks_controller.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 (6)
app/models/product.rb (1)
94-100: change_learning_status は現状呼び出し箇所が存在しないため例外懸念は発生しません 呼び出し元実装時に必要に応じて例外ハンドリングを検討してください。Likely an incorrect or invalid review comment.
config/initializers/active_support_notifications.rb (1)
36-36: LGTM！
check.createイベントに対する新しいサブスクライバーの登録が適切に実装されています。既存のパターンに沿っており、LearningStatusUpdaterとの併用も問題ありません（同一イベントに複数のサブスクライバーを登録することはActiveSupport::Notificationsの標準的な使い方です）。app/models/check_notifier_for_submitter.rb (1)
1-14: LGTM！
CheckNotifierForSubmitterの実装が適切です：
callメソッドのシグネチャがActiveSupport::Notificationsの標準に準拠しています- ガード句により、通知が必要な条件を明確に定義しています（receiverの存在、sender≠receiver、checkable_type≠'Report'）
ActivityDeliveryを使用した通知処理が正しく実装されています過去のレビューコメントより、例外処理はコントローラー側の
safe_instrumentで行う方針のため、この実装で問題ありません。Based on learnings
app/controllers/checks_controller.rb (2)
18-20: LGTM！
check.createイベントの通知処理が適切に実装されています：
safe_instrumentにより例外が捕捉され、ログ出力のみで処理が継続される設計になっています@checkableの型に応じた適切なメッセージが表示されます- 過去の議論で合意された方針（check保存後の副作用で例外が発生してもロールバックしない）に沿った実装です
Based on learnings
30-35: LGTM！
@check.destroyの結果を確認してから通知を送信する実装が正しく追加されています。過去のレビューコメントで指摘された問題が適切に修正されています。app/models/check.rb (1)
6-7: LGTM！コールバックの定義が
:clear_cacheメソッドを使用するように変更され、CheckCallbacksへの依存が削除されています。この変更により、キャッシュクリア処理がCheckモデル内に集約され、コードの見通しが良くなっています。
sjabcdefin
force-pushed
the
chore/replace-check-events-with-notifications
branch
from
f75af79 to
3fb7549
Compare
|
@coderabbitai ご確認お願いいたします。 |
|
@sjabcdefin さん、ご対応ありがとうございます! 前回の指摘事項(Duplicate comments と Nitpick Comments)を修正していただいたとのこと、承知いたしました。変更内容を確認させていただきます。 ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/controllers/checks_controller.rb (1)
50-54: パラメータ名をより明確にすることを検討してください。メソッドのパラメータ名
checkは実際にはハッシュ{ check: @check }を受け取るため、payloadという名前の方がより明確です。以下のdiffを適用してパラメータ名を変更できます:
- def safe_instrument(event, check) - ActiveSupport::Notifications.instrument(event, check) + def safe_instrument(event, payload) + ActiveSupport::Notifications.instrument(event, payload) rescue StandardError => e Rails.logger.warn "[Notifications] '#{event}'サブスクライブ処理でエラーが発生しました。：#{e.message}" end
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/controllers/checks_controller.rb(3 hunks)app/models/check.rb(1 hunks)app/models/check_callbacks.rb(0 hunks)app/models/check_notifier_for_submitter.rb(1 hunks)app/models/product.rb(1 hunks)config/initializers/active_support_notifications.rb(1 hunks)
💤 Files with no reviewable changes (1)
- app/models/check_callbacks.rb
🚧 Files skipped from review as they are similar to previous changes (2)
- app/models/product.rb
- app/models/check_notifier_for_submitter.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/controllers/checks_controller.rbapp/models/check.rbconfig/initializers/active_support_notifications.rb
🧠 Learnings (2)
📓 Common learnings
Learnt from: sjabcdefin
PR: fjordllc/bootcamp#9247
File: app/controllers/checks_controller.rb:18-18
Timestamp: 2025-10-12T03:08:58.326Z
Learning: ChecksController#createでは、check.saveが成功した後の副作用（ステータス更新、アプリ内通知）で例外が発生しても、checkオブジェクトはロールバックせず、ログ出力のみで対応する方針。
📚 Learning: 2025-10-12T03:08:58.326Z
Learnt from: sjabcdefin
PR: fjordllc/bootcamp#9247
File: app/controllers/checks_controller.rb:18-18
Timestamp: 2025-10-12T03:08:58.326Z
Learning: ChecksController#createでは、check.saveが成功した後の副作用（ステータス更新、アプリ内通知）で例外が発生しても、checkオブジェクトはロールバックせず、ログ出力のみで対応する方針。
Applied to files:
app/controllers/checks_controller.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)
config/initializers/active_support_notifications.rb (1)
36-36: LGTM!CheckNotifierForSubmitterの購読が適切に追加されています。LearningStatusUpdaterと並行して動作する構成になっており、Newspaperからの移行方針に沿っています。
app/controllers/checks_controller.rb (2)
18-20: LGTM!safe_instrumentラッパーを使用してイベント通知を行い、例外が発生してもcheckオブジェクトのロールバックは行わない実装になっています。過去の議論で合意した方針に沿っています。
Based on learnings
30-35: LGTM!destroyの成功を確認してから通知を送信する実装になっており、過去のレビューコメントで指摘された問題が適切に修正されています。
app/models/check.rb (2)
6-7: LGTM!クラスベースのコールバックからシンボルベースのコールバックへの移行により、コードがシンプルになっています。Railsの慣例に従った実装です。
18-35: LGTM!キャッシュクリアロジックがモデル内に適切に実装されています。checkable_typeによる分岐も正しく、ReportとProductそれぞれに対応するキャッシュ削除処理が呼ばれています。
sjabcdefin
force-pushed
the
chore/replace-check-events-with-notifications
branch
from
3fb7549 to
b8ff744
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/models/check_notifier_for_submitter.rb (1)
4-6: payload の妥当性チェックを追加してください。
payload[:check]がnilの場合、Line 6 でNoMethodErrorが発生します。instrument 呼び出し側の実装ミスを早期検知するため、ガード句を追加することを推奨します。以下の diff を適用して、payload の妥当性チェックを追加してください：
def call(_name, _started, _finished, _id, payload) check = payload[:check] + return unless check return unless check.receiver && check.sender != check.receiver && check.checkable_type != 'Report'
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/controllers/api/checks_controller.rb(2 hunks)app/controllers/checks_controller.rb(3 hunks)app/models/check.rb(1 hunks)app/models/check_callbacks.rb(0 hunks)app/models/check_notifier_for_submitter.rb(1 hunks)app/models/product.rb(1 hunks)config/initializers/active_support_notifications.rb(1 hunks)
💤 Files with no reviewable changes (1)
- app/models/check_callbacks.rb
🚧 Files skipped from review as they are similar to previous changes (3)
- app/models/check.rb
- app/models/product.rb
- config/initializers/active_support_notifications.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/controllers/checks_controller.rbapp/controllers/api/checks_controller.rbapp/models/check_notifier_for_submitter.rb
🧠 Learnings (2)
📓 Common learnings
Learnt from: sjabcdefin
PR: fjordllc/bootcamp#9247
File: app/controllers/checks_controller.rb:18-18
Timestamp: 2025-10-12T03:08:58.343Z
Learning: ChecksController#createでは、check.saveが成功した後の副作用（ステータス更新、アプリ内通知）で例外が発生しても、checkオブジェクトはロールバックせず、ログ出力のみで対応する方針。
📚 Learning: 2025-10-12T03:08:58.343Z
Learnt from: sjabcdefin
PR: fjordllc/bootcamp#9247
File: app/controllers/checks_controller.rb:18-18
Timestamp: 2025-10-12T03:08:58.343Z
Learning: ChecksController#createでは、check.saveが成功した後の副作用（ステータス更新、アプリ内通知）で例外が発生しても、checkオブジェクトはロールバックせず、ログ出力のみで対応する方針。
Applied to files:
app/controllers/checks_controller.rbapp/controllers/api/checks_controller.rb
🧬 Code graph analysis (1)
app/models/check_notifier_for_submitter.rb (1)
app/models/check.rb (1)
receiver(12-14)
⏰ 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 (4)
app/controllers/checks_controller.rb (2)
50-54: LGTM！安全な通知処理が実装されています。
safe_instrumentメソッドにより、通知処理の例外が適切に捕捉され、ログ出力されています。eventパラメータを使用することで、check.createとcheck.cancelの両方のイベントで正確なログが出力される実装になっています。Based on learnings
30-35: destroy の成功確認後に通知を送信する実装が適切です。
@check.destroyの戻り値を確認してからsafe_instrumentを呼び出すことで、実際に削除されたチェックに対してのみサブスクライバーが処理を実行するようになっています。失敗時のエラーハンドリングも適切です。app/controllers/api/checks_controller.rb (2)
38-42: LGTM！Web コントローラと同じ安全な通知処理が実装されています。
safe_instrumentメソッドの実装により、通知処理の例外が適切に捕捉され、ログ出力されています。API 応答に影響を与えずに通知エラーを処理できる設計になっています。Based on learnings
26-27: destroy! 使用時のコールバック失敗を考慮してください。
destroy!はバリデーション失敗時だけでなく、コールバック（before_destroyなど）でfalseを返した場合や例外が発生した場合にもActiveRecord::RecordNotDestroyedを raise します。現在の実装では、コールバック失敗時に例外が API クライアントに伝播しますが、これが意図した動作か確認してください。Web コントローラでは
destroyを使用し、失敗時は alert 付きでリダイレクトしています。API でも同様に、削除失敗を制御したい場合は以下のパターンを検討してください：@check = Check.find(params[:id]) if @check.destroy safe_instrument('check.cancel', check: @check) head :no_content else render json: { message: 'チェックの削除に失敗しました。' }, status: :unprocessable_entity end現在の
destroy!を使用するアプローチが意図的（削除失敗時は 500 エラーとして扱う）であれば、このコメントは無視してください。
| ActivityDelivery.with( | ||
| sender: check.sender, | ||
| receiver: check.receiver, | ||
| check: | ||
| ).notify(:checked) |
There was a problem hiding this comment.
ActivityDelivery の例外を捕捉してログ出力してください。
ActivityDelivery.notify 内部で Notification.create! が呼ばれるため、DB エラーやバリデーションエラー時に例外が発生します。学習済みの方針に従い、check オブジェクトの保存は成功しているため、通知失敗時は例外を伝播させず、ログ出力のみで対応すべきです。
Based on learnings
以下の diff を適用して、例外処理を追加してください：
- ActivityDelivery.with(
- sender: check.sender,
- receiver: check.receiver,
- check:
- ).notify(:checked)
+ ActivityDelivery.with(
+ sender: check.sender,
+ receiver: check.receiver,
+ check:
+ ).notify(:checked)
+ rescue StandardError => e
+ Rails.logger.warn "[CheckNotifierForSubmitter] 通知作成でエラーが発生しました。check_id=#{check.id}, error=#{e.message}"
end
end🤖 Prompt for AI Agents
In app/models/check_notifier_for_submitter.rb around lines 8 to 12, wrap the
ActivityDelivery.with(...).notify(:checked) call in a begin/rescue that catches
StandardError (or more specific errors if desired) and logs the failure instead
of re-raising; include useful context such as the check.id (or other
identifiers), the exception message and backtrace in the log (e.g.,
Rails.logger.error "ActivityDelivery.notify failed for check=#{check.id}:
#{e.message}\n#{e.backtrace.join("\n")}"), and do not propagate the exception so
normal flow continues.
There was a problem hiding this comment.
app/models/check_notifier_for_submitter.rbの呼び出し元であるapp/controllers/checks_controller.rbやapp/controllers/api/checks_controller.rbで例外を捕捉し、ログ出力している。
上記方針で問題があるでしょうか？
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# コントローラー側のsafe_instrumentメソッドの実装を確認
echo "=== ChecksController の safe_instrument 実装 ==="
rg -nA 10 "def safe_instrument" app/controllers/checks_controller.rb
echo -e "\n=== API::ChecksController の safe_instrument 実装 ==="
rg -nA 10 "def safe_instrument" app/controllers/api/checks_controller.rb
echo -e "\n=== safe_instrumentの呼び出し箇所 ==="
rg -n "safe_instrument" app/controllers/checks_controller.rb app/controllers/api/checks_controller.rb
Length of output: 1133
@sjabcdefin さん、コントローラー側での例外処理で問題ありません。むしろ良い設計だと思います。
現在の実装が妥当な理由：
-
ActiveSupport::Notifications の動作特性
ActiveSupport::Notifications.instrumentは同期的に実行されるため、subscriber（CheckNotifierForSubmitter）で発生した例外は確実にinstrumentの呼び出し元に伝播します- つまり、コントローラー側の
safe_instrumentで捕捉できます
-
設計上の利点
- 例外処理を一箇所（コントローラー）で管理できるため、保守性が高い
- 将来
check.createイベントに複数の subscriber を追加しても、コントローラー側の処理で一括してハンドリングできる - subscriber 側の実装がシンプルになり、責務が明確になる
-
学習済みの方針との整合性
- check.save 成功後の副作用で例外が発生してもログ出力のみで対応する方針に従っており、一貫性がある
補足：
より詳細なログが必要な場合は、subscriber 側でも rescue して詳細情報（例：check.id, check.sender.id など）をログ出力した後に raise で再送出することも可能ですが、現状のログメッセージにはイベント名が含まれているため、トラブルシューティングには十分と考えられます。
したがって、私の当初のコメントは誤りでした。現在の実装方針で進めてください。
🧠 Learnings used
Learnt from: sjabcdefin
PR: fjordllc/bootcamp#9247
File: app/controllers/checks_controller.rb:18-18
Timestamp: 2025-10-12T03:08:58.343Z
Learning: ChecksController#createでは、check.saveが成功した後の副作用（ステータス更新、アプリ内通知）で例外が発生しても、checkオブジェクトはロールバックせず、ログ出力のみで対応する方針。
Learnt from: jun-kondo
PR: fjordllc/bootcamp#0
File: :0-0
Timestamp: 2025-07-23T21:11:21.826Z
Learning: fjordllc/bootcampプロジェクトの`app/models/inquiry_notifier.rb`において、`NOTIFICATION_SENDER_NAME`で指定される管理者ユーザーは必ず存在することがシステムの前提条件として保証されているため、senderがnilになることは想定されておらず、エラーハンドリングは不要である。
There was a problem hiding this comment.
例外ハンドリングに関する指摘の修正方針変更により、新規に作成していたapp/models/check_notifier_for_submitter.rbは削除いたしました。
There was a problem hiding this comment.
@sjabcdefin さん、了解しました。check_notifier_for_submitter.rb の削除を確認しました。
例外ハンドリングの方針変更により、別のアプローチを採用されたとのことですね。もし新しい実装について確認が必要でしたら、お気軽にレビューをご依頼ください。
PRの他の部分で何かお手伝いできることがあれば、お知らせください。
🧠 Learnings used
Learnt from: jun-kondo
PR: fjordllc/bootcamp#0
File: :0-0
Timestamp: 2025-07-23T21:11:21.826Z
Learning: fjordllc/bootcampプロジェクトの`app/models/inquiry_notifier.rb`において、`NOTIFICATION_SENDER_NAME`で指定される管理者ユーザーは必ず存在することがシステムの前提条件として保証されているため、senderがnilになることは想定されておらず、エラーハンドリングは不要である。
Learnt from: jun-kondo
PR: fjordllc/bootcamp#0
File: :0-0
Timestamp: 2025-07-23T21:11:21.826Z
Learning: fjordllc/bootcampプロジェクトの`app/models/work_notification_destroyer.rb`において、このクラスは`app/controllers/works_controller.rb`の`destroy`アクションから呼び出され、`before_action :set_my_work`でIDの妥当性チェックが事前に行われるため、payloadのworkオブジェクトは常に有効であることが保証されている。そのためエラーハンドリングは不要。
Learnt from: sjabcdefin
PR: fjordllc/bootcamp#9247
File: app/controllers/checks_controller.rb:0-0
Timestamp: 2025-10-22T06:04:36.027Z
Learning: ChecksController#createおよび#destroyでは、Checkの作成・削除とActiveSupport::Notifications.instrumentによるイベント発行(プラクティスのステータス更新)を同一トランザクション内で実行し、いずれかが失敗した場合は両方をロールバックする。これによりWebUI表示とDB状態の整合性を保証している。
Learnt from: reckyy
PR: fjordllc/bootcamp#8356
File: app/models/notification.rb:45-47
Timestamp: 2025-07-15T12:58:02.735Z
Learning: fjordllc/bootcampプロジェクトの`app/models/notification.rb`において、`training_completed`通知種別は`TARGETS_TO_KINDS`ハッシュに含めない設計になっている。これは`training_completed`が通知一覧のフィルタ対象ではないためで、意図的な設計判断である。
b8ff744 to
c6f1a97
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
app/models/check_callbacks.rb (1)
6-14: 通知エラーのログに詳細情報を追加してください。エラーメッセージのみをログ出力していますが、デバッグ時に以下の情報があると問題の特定が容易になります：
check.idcheck.checkable_type/check.checkable_idcheck.sender_id/check.receiver_id- バックトレース（
e.backtrace.first(5)など）以下の差分を適用して、ログの詳細度を向上させることを検討してください：
rescue StandardError => e - Rails.logger.warn("[CheckCallbacks] 通知処理でエラー: #{e.message}") + Rails.logger.warn( + "[CheckCallbacks] 通知処理でエラー: #{e.message} " \ + "(check_id: #{check.id}, checkable: #{check.checkable_type}##{check.checkable_id}, " \ + "sender_id: #{check.sender_id}, receiver_id: #{check.receiver&.id})" + ) + Rails.logger.warn(e.backtrace.first(5).join("\n")) if e.backtrace end
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/controllers/api/checks_controller.rb(1 hunks)app/controllers/checks_controller.rb(2 hunks)app/models/check.rb(1 hunks)app/models/check_callbacks.rb(1 hunks)app/models/product.rb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/models/product.rb
- app/models/check.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/controllers/checks_controller.rbapp/models/check_callbacks.rbapp/controllers/api/checks_controller.rb
🧠 Learnings (2)
📓 Common learnings
Learnt from: matuaya
PR: fjordllc/bootcamp#9108
File: app/models/after_user_retirement.rb:0-0
Timestamp: 2025-09-01T22:31:57.345Z
Learning: fjordllc/bootcampプロジェクトにおいて、`Newspaper.publish(:retirement_create, { user: user })`は通知処理ではなく、UnfinishedDataDestroyerやTimesChannelDestroyer等による重要なデータクリーンアップ処理を実行するため、管理者による退会（triggered_by: 'admin'）の場合でも必ず実行する必要がある。
Learnt from: matuaya
PR: fjordllc/bootcamp#9108
File: app/models/after_user_retirement.rb:0-0
Timestamp: 2025-09-01T22:31:57.345Z
Learning: fjordllc/bootcampプロジェクトにおいて、`Newspaper.publish(:retirement_create, { user: user })`はUnfinishedDataDestroyer（未チェックProduct・WIP Report削除、career_pathリセット）とTimesChannelDestroyer（Discord分報チャンネル削除）による重要なデータクリーンアップ処理を実行するため、退会のトリガーが何であろうと（admin、hibernation、user）必ず実行する必要がある。
📚 Learning: 2025-10-12T03:08:58.343Z
Learnt from: sjabcdefin
PR: fjordllc/bootcamp#9247
File: app/controllers/checks_controller.rb:18-18
Timestamp: 2025-10-12T03:08:58.343Z
Learning: ChecksController#createでは、check.saveが成功した後の副作用（ステータス更新、アプリ内通知）で例外が発生しても、checkオブジェクトはロールバックせず、ログ出力のみで対応する方針。
Applied to files:
app/controllers/checks_controller.rbapp/models/check_callbacks.rbapp/controllers/api/checks_controller.rb
🧬 Code graph analysis (1)
app/models/check_callbacks.rb (1)
app/models/check.rb (1)
receiver(12-14)
⏰ 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)
app/controllers/checks_controller.rb (1)
17-26: トランザクション設計の確認：意図した動作になっています。過去のレビューコメントでの議論を踏まえ、以下の設計が正しく実装されていることを確認しました：
- ✅ Check の作成とステータス更新（
LearningStatusUpdater経由）が同一トランザクション内で実行され、いずれかが失敗すると両方ロールバック- ✅ 通知処理（
CheckCallbacks#after_create内のActivityDelivery）は例外を捕捉してログ出力のみ行い、トランザクションに影響しない- ✅
Product#change_learning_statusがupdate!を使用し、ステータス更新の失敗が適切に例外として伝播この設計により、UI表示に影響するステータス更新の整合性が保証される一方、副次的な通知処理の失敗がシステム全体に波及しない適切なバランスが取れています。
Based on learnings
| begin | ||
| Check.transaction do | ||
| @check = Check.create!(user: current_user, checkable:) | ||
| ActiveSupport::Notifications.instrument('check.create', check: @check) | ||
| end | ||
| head :created | ||
| rescue StandardError | ||
| render json: { message: 'エラーが発生しました。' } | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
エラー時のログ出力を追加してください。
rescue StandardError で例外を捕捉していますが、エラー内容をログに記録していないため、障害発生時の原因特定が困難です。ユーザーには汎用的なエラーメッセージを返しつつ、ログには詳細な情報を記録することを推奨します。
以下の差分を適用して、エラーログを追加してください：
- rescue StandardError
+ rescue StandardError => e
+ Rails.logger.error(
+ "[API::ChecksController#create] チェック作成でエラー: #{e.message} " \
+ "(checkable: #{params[:checkable_type]}##{params[:checkable_id]}, user_id: #{current_user.id})"
+ )
+ Rails.logger.error(e.backtrace.first(10).join("\n")) if e.backtrace
render json: { message: 'エラーが発生しました。' }
end🤖 Prompt for AI Agents
In app/controllers/api/checks_controller.rb around lines 14 to 22, the rescue
block swallows exceptions without logging; change the rescue to capture the
exception (rescue StandardError => e) and add a server-side error log (e.g.
Rails.logger.error or logger.error) that includes the exception message and
backtrace (and optional context such as current_user.id and checkable) before
rendering the generic JSON error to the client.
There was a problem hiding this comment.
@sjabcdefin さん、エラーログの追加をありがとうございます！
例外のキャッチとログ出力が適切に実装されていることを確認しました。1点だけ気づいた点があります：
ログメッセージ内のコントローラ名が [ChecksController#create] となっていますが、このファイルは API::ChecksController なので、以下のように修正するとより正確になります：
create メソッド（Line 20）:
Rails.logger.error("[API::ChecksController#create] チェック作成でエラー: #{e.message}")destroy メソッド（Line 32）:
Rails.logger.error("[API::ChecksController#destroy] チェック削除でエラー: #{e.message}")これにより、ログからエラーの発生箇所を特定しやすくなります。
/\_/\
( o.o )
> ^ < ナイスです！
/| |\
(| |)
🧠 Learnings used
Learnt from: sjabcdefin
PR: fjordllc/bootcamp#9247
File: app/controllers/checks_controller.rb:0-0
Timestamp: 2025-10-22T06:04:36.027Z
Learning: ChecksController#createおよび#destroyでは、Checkの作成・削除とActiveSupport::Notifications.instrumentによるイベント発行(プラクティスのステータス更新)を同一トランザクション内で実行し、いずれかが失敗した場合は両方をロールバックする。これによりWebUI表示とDB状態の整合性を保証している。
Learnt from: Miya096jp
PR: fjordllc/bootcamp#9102
File: app/controllers/mentor/buzzes_controller.rb:0-0
Timestamp: 2025-09-04T01:39:22.261Z
Learning: fjordllc/bootcamp プロジェクトでは、更新処理の成功時の通知メッセージについて、I18n を使用せずに日本語文字列を直接記述する方針で一貫性を保っている。
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
There was a problem hiding this comment.
[API::ChecksController#create], [API::ChecksController#destroy]に修正いたしました。
ご確認お願いいたします。
コミット：459fd65
There was a problem hiding this comment.
@sjabcdefin さん、修正ありがとうございます！
ログメッセージのコントローラ名が [API::ChecksController#create] と [API::ChecksController#destroy] に正しく更新されていることを確認しました。これでログから発生箇所を正確に特定できるようになりました。完璧です！✨
/\_/\
( ^.^ )
> ♥ < 完璧です！
/| |\
(| |)
🧠 Learnings used
Learnt from: sjabcdefin
PR: fjordllc/bootcamp#9247
File: app/controllers/checks_controller.rb:0-0
Timestamp: 2025-10-22T06:04:36.027Z
Learning: ChecksController#createおよび#destroyでは、Checkの作成・削除とActiveSupport::Notifications.instrumentによるイベント発行(プラクティスのステータス更新)を同一トランザクション内で実行し、いずれかが失敗した場合は両方をロールバックする。これによりWebUI表示とDB状態の整合性を保証している。
| def destroy | ||
| @check = Check.find(params[:id]).destroy | ||
| ActiveSupport::Notifications.instrument('check.cancel', check: @check) | ||
|
|
||
| Check.transaction do | ||
| @check = Check.find(params[:id]).destroy! | ||
| ActiveSupport::Notifications.instrument('check.cancel', check: @check) | ||
| end | ||
| head :no_content | ||
| rescue StandardError | ||
| render json: { message: 'エラーが発生しました。' } | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
エラー時のログ出力を追加してください。
create アクションと同様に、destroy アクションでも例外の詳細をログに記録することで、障害対応が容易になります。
以下の差分を適用して、エラーログを追加してください：
rescue StandardError => e
+ Rails.logger.error(
+ "[API::ChecksController#destroy] チェック削除でエラー: #{e.message} " \
+ "(check_id: #{params[:id]}, user_id: #{current_user.id})"
+ )
+ Rails.logger.error(e.backtrace.first(10).join("\n")) if e.backtrace
render json: { message: 'エラーが発生しました。' }
endCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In app/controllers/api/checks_controller.rb around lines 28 to 36, the destroy
action rescues StandardError but does not log the exception details; update the
rescue to capture the exception (e.g., rescue StandardError => e) and log the
error message and backtrace (using Rails.logger.error or logger.error) before
rendering the JSON error response so operators have the exception context for
troubleshooting.
There was a problem hiding this comment.
@sjabcdefin さん、修正ありがとうございます！確認いたしました。
エラーログの追加が正しく実装されています。destroy アクションでも StandardError => e で例外をキャッチし、Rails.logger.error でログ出力するようになりました。また、create アクションと同じログ形式で統一されているため、コードの一貫性も保たれています。
これで、エラー発生時の調査がしやすくなります。対応お疲れ様でした！
🧠 Learnings used
Learnt from: sjabcdefin
PR: fjordllc/bootcamp#9247
File: app/controllers/checks_controller.rb:0-0
Timestamp: 2025-10-22T06:04:36.027Z
Learning: ChecksController#createおよび#destroyでは、Checkの作成・削除とActiveSupport::Notifications.instrumentによるイベント発行(プラクティスのステータス更新)を同一トランザクション内で実行し、いずれかが失敗した場合は両方をロールバックする。これによりWebUI表示とDB状態の整合性を保証している。
Learnt from: Miya096jp
PR: fjordllc/bootcamp#9102
File: app/controllers/mentor/buzzes_controller.rb:0-0
Timestamp: 2025-09-04T01:39:22.261Z
Learning: fjordllc/bootcamp プロジェクトでは、更新処理の成功時の通知メッセージについて、I18n を使用せずに日本語文字列を直接記述する方針で一貫性を保っている。
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
sjabcdefin
force-pushed
the
chore/replace-check-events-with-notifications
branch
2 times, most recently
from
2b0fa50 to
459fd65
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/controllers/checks_controller.rb (1)
34-43: destroyアクションも同様にエラーログの改善が推奨されます。
createアクションと同様に、過去のレビューコメントでエラーログの詳細化が提案されています。以下の情報を追加することで、障害発生時の原因特定が迅速化されます：
check_idcheckableの型とIDuser_id- スタックトレース
過去コメントの提案を適用し、
createとdestroyで一貫したエラーログ出力を実現することをご検討ください。
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/controllers/api/checks_controller.rb(1 hunks)app/controllers/checks_controller.rb(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/controllers/api/checks_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/controllers/checks_controller.rb
🧠 Learnings (4)
📓 Common learnings
Learnt from: sjabcdefin
PR: fjordllc/bootcamp#9247
File: app/controllers/checks_controller.rb:0-0
Timestamp: 2025-10-22T06:04:36.027Z
Learning: ChecksController#createおよび#destroyでは、Checkの作成・削除とActiveSupport::Notifications.instrumentによるイベント発行(プラクティスのステータス更新)を同一トランザクション内で実行し、いずれかが失敗した場合は両方をロールバックする。これによりWebUI表示とDB状態の整合性を保証している。
Learnt from: sjabcdefin
PR: fjordllc/bootcamp#9247
File: app/controllers/checks_controller.rb:0-0
Timestamp: 2025-10-22T06:04:36.027Z
Learning: CheckCallbacksはafter_create_commitで実行されるため、Checkのトランザクションがコミットされた後に動作する。CheckCallbacks内のメール通知・サイト内通知でエラーが発生した場合は、副次的な処理とみなし、例外を発生させずワーニングログのみ出力する設計になっている。
Learnt from: matuaya
PR: fjordllc/bootcamp#9108
File: app/models/after_user_retirement.rb:0-0
Timestamp: 2025-09-01T22:31:57.345Z
Learning: fjordllc/bootcampプロジェクトにおいて、`Newspaper.publish(:retirement_create, { user: user })`は通知処理ではなく、UnfinishedDataDestroyerやTimesChannelDestroyer等による重要なデータクリーンアップ処理を実行するため、管理者による退会（triggered_by: 'admin'）の場合でも必ず実行する必要がある。
📚 Learning: 2025-10-22T06:04:36.027Z
Learnt from: sjabcdefin
PR: fjordllc/bootcamp#9247
File: app/controllers/checks_controller.rb:0-0
Timestamp: 2025-10-22T06:04:36.027Z
Learning: ChecksController#createおよび#destroyでは、Checkの作成・削除とActiveSupport::Notifications.instrumentによるイベント発行(プラクティスのステータス更新)を同一トランザクション内で実行し、いずれかが失敗した場合は両方をロールバックする。これによりWebUI表示とDB状態の整合性を保証している。
Applied to files:
app/controllers/checks_controller.rb
📚 Learning: 2025-10-22T06:04:36.027Z
Learnt from: sjabcdefin
PR: fjordllc/bootcamp#9247
File: app/controllers/checks_controller.rb:0-0
Timestamp: 2025-10-22T06:04:36.027Z
Learning: CheckCallbacksはafter_create_commitで実行されるため、Checkのトランザクションがコミットされた後に動作する。CheckCallbacks内のメール通知・サイト内通知でエラーが発生した場合は、副次的な処理とみなし、例外を発生させずワーニングログのみ出力する設計になっている。
Applied to files:
app/controllers/checks_controller.rb
📚 Learning: 2025-09-04T01:39:22.261Z
Learnt from: Miya096jp
PR: fjordllc/bootcamp#9102
File: app/controllers/mentor/buzzes_controller.rb:0-0
Timestamp: 2025-09-04T01:39:22.261Z
Learning: fjordllc/bootcamp プロジェクトでは、更新処理の成功時の通知メッセージについて、I18n を使用せずに日本語文字列を直接記述する方針で一貫性を保っている。
Applied to files:
app/controllers/checks_controller.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)
app/controllers/checks_controller.rb (1)
17-27: トランザクション実装は適切です。エラーログの改善提案が過去コメントにあります。Based on learnings: トランザクション内で
save!とイベント発行を実行する実装は、DB状態とWebUI表示の整合性を保証する設計として適切です。ただし、過去のレビューコメントで提案されているように、エラーログに以下の情報を追加することで障害分析が容易になります：
checkableの型とIDuser_id- スタックトレース（バックトレース）
過去コメントの提案を適用することをご検討ください。
|
お疲れ様です！ また、FjordChoiceに、引き受けたレビューが反映されない場合があるようです。 |
|
@sjabcdefin |
|
お疲れさまです！ |
yokomaru
left a comment
yokomaru
left a comment
There was a problem hiding this comment.
@sjabcdefin
お疲れ様です！指定した期限より少し遅くなってしまい申し訳ありません…！🙇♀️
動作確認いたしましたが、問題ありませんでした！
検証項目がとても丁寧で流れも分かりやすかったので、とても検証しやすかったです 🙌
1点だけ軽くコメントさせていただきましたが、個人的に気になった程度の内容なので、最終的な判断はお任せします！
また、トランザクションや例外処理まわりのハンドリングも非常に勉強になりました！
例外処理関連のコミットメッセージが少しシンプル（ChecksController の例外処理を改善）に感じたので、以下あたりの変更内容や背景（Why）などどのように改善されたかを残していただけたら、後から履歴を追いやすいかなと思いました 💡
after_create_commit/update!への変更の理由rescue StandardErrorを採用した背景rescue StandardErrorはかなり広い範囲のエラーを捕捉する認識なのですが、メンターの方や CodeRabbit とのやり取りを踏まえた上でこの形になっていると思うので、その経緯や理由を軽く残していただくと、後からコードを読む方にも意図が伝わりやすいかなと思いました 💡
※ CodeRabbit とのやり取りを拝見し、議論を踏まえた実装だと伝わってきたので、その経緯がコミットログに軽く残っているとより親切かも！というくらいの温度感です 🙏
お手数おかけいたしますが、よろしくお願いいたします。
| head :created | ||
| rescue StandardError => e | ||
| Rails.logger.error("[API::ChecksController#create] チェック作成でエラー: #{e.message}") | ||
| render json: { message: 'エラーが発生しました。' } |
There was a problem hiding this comment.
[IMO]
renderはステータスを指定しない場合、デフォルトで200(成功)を返すので、
StandardError が発生している＝サーバー側の処理に失敗した状態でレスポンスを返す場合は、 status: :internal_server_errorなどのエラーステータスを付与した方がAPIとして適切かな？と思ったのですが、tomokaさん的にどう思われますかっ？
※API を叩いているJS 側(comment-chekable.js)では、HTTP ステータスではなくmessage の有無でエラーを判定しているようなので、現状このステータスがあってもなくても動作上は問題なさそうなため最終的な判断はお任せいたします！🙏
There was a problem hiding this comment.
ご指摘ありがとうございます！
エラー時に200(成功)を返すのは不適切だと思い、status: :internal_server_errorを返すように修正いたしました。😌🙏
コミット：05be5d6
過去のissue対応での置き換え漏れを修正
- 画面上の不整合が発生しないよう、チェック保存/削除とステータス更新を同一トランザクション内で実行し、 いずれかが失敗した場合はロールバックされるように変更 - サイト内通知・メール通知は、画面上の不整合が起きないため、トランザクション外での実行でも問題ないと判断 - 例外を検知できるよう、update / save を update! / save! に変更 - トランザクション完了後にコールバックを実行するため、 after_create / after_destroy を after_create_commit / after_destroy_commit に変更
- 画面上の不整合が発生しないよう、チェック保存/削除とステータス更新を同一トランザクション内で実行し、 いずれかが失敗した場合はロールバックされるように変更 - サイト内通知・メール通知は、画面上の不整合が起きないため、トランザクション外での実行でも問題ないと判断 - 例外を検知できるよう、destroy を destroy! に変更
例外発生時に200を返すのは不適切なため 削除処理については現状使用されていないが併せて同様の対応を実施
sjabcdefin
force-pushed
the
chore/replace-check-events-with-notifications
branch
from
459fd65 to
05be5d6
Compare
yokomaru
left a comment
yokomaru
left a comment
There was a problem hiding this comment.
お疲れ様です！ご対応いただきありがとうございました〜！🙌
コミットメッセージ、丁寧にまとめられていてとても読みやすく、非常に勉強になりました！
こちらApproveさせていただきます！！
引き続きどうぞよろしくお願いいたします！！
|
@yokomaru @okuramasafumi |
|
@okuramasafumi @komagata |
|
@sjabcdefin マージしました〜 |
4 participants
Issue
概要
ChecksController内のnewspaperをActiveSupport::Notificationsに置き換え※ 経緯については、:check_createと:check_cancelの件について を参照
WebUI上の表示とDB状態の不整合を防ぐため、
ChecksController#createおよびChecksController#destroyにおいて、以下の処理を同一トランザクション内で実行し、いずれかが失敗した場合は両方をロールバックするように変更Checkの作成・削除ActiveSupport::Notifications.instrumentによるイベント発行(プラクティスのステータス更新)※issueに記載がある最終作業については、newspaper に関する Issue が全てマージされていないため、別issueにて登録予定です。
変更確認方法
chore/replace-check-events-with-notificationsをローカルに取り込むforeman start -f Procfile.devでローカル環境を立ち上げる提出物の確認(
ChecksController)komagataさんでログインするPC性能の見方を知るの提出物にアクセスする
提出物欄の「提出物を合格にする」ボタンをクリックする
「提出物を合格にしました。」メッセージが表示されることを確認する
kimuraさんでログインし、以下を確認するkomagataさんでログインするPC性能の見方を知るの提出物にアクセスする
提出物欄の「提出物の合格を取り消す」をクリックする
kimuraさんでログインし、以下を確認する提出物の確認(
API::ChecksController)komagataさんでログインするkimuraさんでログインし、以下を確認する日報の確認(
ChecksController)komagataさんでログインする日報の確認(
API::ChecksController)komagataさんでログインするScreenshot
Summary by CodeRabbit
Summary by CodeRabbit
リリースノート
バグ修正
改善