Conversation
Walkthroughコントローラをネームスペース付きクラス宣言へ変更し基底を SchedulerController に差し替え。QuestionAutoCloser から既存ベストアンサー削除処理を除去してクローズフローを簡素化。自動クローズ検証用の質問と回答フィクスチャを各1件追加。 Changes
Sequence Diagram(s)sequenceDiagram
participant Scheduler as Scheduler::Daily::AutoCloseQuestionsController
participant Closer as QuestionAutoCloser
participant Answer
participant Events as EventPublisher
Scheduler->>Closer: show -> post_warning, close_and_select_best_answer
activate Closer
Closer->>Answer: create_close_message(question)
Closer->>Answer: select_as_best_answer(close_message)
Closer->>Events: publish_events(question, close_message)
deactivate Closer
Note over Closer: 既存ベストアンサー削除の呼び出しは廃止
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related issues
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/scheduler/daily/auto_close_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. ✨ 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: 0
🧹 Nitpick comments (2)
db/fixtures/questions.yml (1)
135-142: （任意）updated_at の明示や、時間依存のテスト安定性について一言。時間ベースの fixture は運用上便利ですが、将来「最終活動時刻」を updated_at に依存する変更が入ると判定がぶれる可能性があります。必要に応じて updated_at の明示や、読み込み時刻基準の安定化をご検討ください。
db/fixtures/answers.yml (1)
45-50: 警告メッセージ文言は定数を参照して重複回避を。QuestionAutoCloser の検知ロジックはメッセージ文言に依存するため、将来の文言変更に追従漏れがあると fixture が崩れます。ERB で定数参照にするのが安全です。
適用例（該当範囲のみ）:
-answer51: - description: このQ&Aは1ヶ月間コメントがありませんでした。1週間後に自動的にクローズされます。 +answer51: + description: <%= QuestionAutoCloser::AUTO_CLOSE_WARNING_MESSAGE %> user: pjord question: question51 created_at: <%= Time.current - 1.week %> updated_at: <%= Time.current - 1.week %>
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/controllers/scheduler/daily/auto_close_questions_controller.rb(1 hunks)app/models/question_auto_closer.rb(0 hunks)db/fixtures/answers.yml(1 hunks)db/fixtures/questions.yml(1 hunks)
💤 Files with no reviewable changes (1)
- app/models/question_auto_closer.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/scheduler/daily/auto_close_questions_controller.rb
🧬 Code Graph Analysis (1)
app/controllers/scheduler/daily/auto_close_questions_controller.rb (1)
app/models/question_auto_closer.rb (3)
post_warning(3-105)post_warning(12-21)close_and_select_best_answer(23-32)
🔇 Additional comments (4)
app/controllers/scheduler/daily/auto_close_questions_controller.rb (2)
4-7: LGTM: 処理順と応答は問題なし。
- post_warning → close_and_select_best_answer の順は意図に沿っています（直近で警告を出したものは同一リクエスト内でクローズ対象にならない）。
- head :ok で十分です。例外は 500 を返す運用でも良いですが、必要に応じてメトリクス/ログの追加をご検討ください。
3-7: CSRF無効化とトークン検証フィルターを確認しましたapp/controllers/scheduler_controller.rb にて以下の設定が存在します：
skip_before_action :verify_authenticity_tokenbefore_action :require_tokenこれにより、Scheduler名前空間以下の全エンドポイントでCSRF保護が無効化されつつ、tokenパラメータによる認証が確実に実行されることを確認できました。追加対応は不要です。
db/fixtures/questions.yml (1)
135-142: LGTM: 自動クローズ検証用の fixture 追加が適切。
- 作成/公開時刻が「1ヶ月＋1週間前」で、warning → close の条件を満たすシナリオになっています。
- user/practice の参照も既存データと整合していそうです。
db/fixtures/answers.yml (1)
45-50: 確認済み:pjordユーザー定義とメッセージ文言が一致しています
db/fixtures/users.yml内にpjord:が定義されており、login_name: pjordとして登録済みです。QuestionAutoCloser::SYSTEM_USER_LOGINも'pjord'で一致しています。AUTO_CLOSE_WARNING_MESSAGEの定義（'このQ&Aは1ヶ月間コメントがありませんでした。1週間後に自動的にクローズされます。'）と、fixture のdescriptionの文言が完全に一致しています。これ以上の修正は不要です。
ryufuta
force-pushed
the
bug/question-not-auto-close
branch
from
55db072 to
03d848a
Compare
ryufuta
force-pushed
the
bug/question-not-auto-close
branch
from
03d848a to
4b5763a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/controllers/scheduler/daily/auto_close_questions_controller.rb (2)
4-7: 運用時の観測性を高めるために簡易ログを追加しませんか？ジョブの開始・終了ログがあると障害一次切り分けが容易になります（CloudWatchやPapertrail等の集計でも有用）。
適用例（このファイル内だけの変更で完結）:
def show - QuestionAutoCloser.post_warning - QuestionAutoCloser.close_and_select_best_answer - head :ok + Rails.logger.info('[Scheduler] AutoCloseQuestions start') + QuestionAutoCloser.post_warning + QuestionAutoCloser.close_and_select_best_answer + Rails.logger.info('[Scheduler] AutoCloseQuestions finish') + head :ok end
4-7: GETで副作用を伴うエンドポイントはキャッシュやログの副作用に注意。可能ならPOST+ヘッダ認証へ。スケジューラからの叩きやすさを重視してGETにしていると推測しますが、REST上はPOSTの方が自然で、プロキシキャッシュやリンクプリフェッチの誤作動回避、クエリパラメータに埋めたTOKENのログ混入リスク低減（ヘッダ化）にもつながります。実運用に支障がなければ、将来的にPOST化＋トークンはHeader化を検討してください。
📜 Review details
Configuration used: .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/scheduler/daily/auto_close_questions_controller.rb(1 hunks)app/models/question_auto_closer.rb(0 hunks)db/fixtures/answers.yml(1 hunks)db/fixtures/questions.yml(1 hunks)
💤 Files with no reviewable changes (1)
- app/models/question_auto_closer.rb
🚧 Files skipped from review as they are similar to previous changes (2)
- db/fixtures/answers.yml
- db/fixtures/questions.yml
🧰 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/scheduler/daily/auto_close_questions_controller.rb
🧬 Code Graph Analysis (1)
app/controllers/scheduler/daily/auto_close_questions_controller.rb (1)
app/models/question_auto_closer.rb (3)
post_warning(3-105)post_warning(12-21)close_and_select_best_answer(23-32)
⏰ 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 (2)
app/controllers/scheduler/daily/auto_close_questions_controller.rb (2)
4-7: LGTM：呼び出し順と副作用の分離が妥当です。先に警告を投下し、その後にクローズ判定を行う順序は要件に合致しており、
should_close?が1週間経過判定を持つため同一リクエスト内での誤クローズも防げています。head :okでの軽量レスポンスも妥当です。
3-3: 名前空間付きクラス定義は正常に設定済み。継承とルーティングの整合性を確認完了。検証結果により以下が確認できました：
SchedulerControllerがトップレベルに存在し、ApplicationControllerを継承している- トークン認証が適切に設定されている（
before_action :require_token）- ルーティングで
resource :auto_close_questions, only: %i(show)として singular resource で正しく定義されており、/scheduler/daily/auto_close_questionsが:idなしでshowアクションにマッピングされている名前空間付きクラス定義
class Scheduler::Daily::AutoCloseQuestionsController < SchedulerControllerは Rails の autoload 規約に適合しており、継承先とルーティングの整合性も問題ありません。
| QuestionAutoCloser.post_warning | ||
| QuestionAutoCloser.close_and_select_best_answer |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
QuestionAutoCloser内部のN+1/並行実行による重複発生の可能性を軽減できるか確認を。
呼び出し先では Question.not_solved.find_each + question.answers.select/any? でRuby側フィルタを行っており、大量件数でN+1やメモリ負荷になり得ます。また同時起動時に警告コメントの重複生成が理論上は起こり得ます（判定→作成が非原子的）。性能・整合性要件次第ですが、将来的に以下の改善余地があります。
- 予告/クローズ対象の絞り込みをSQL側に寄せる（LIKE/正規表現・サブクエリ・EXISTS活用）＋
includes(:answers)等で事前ロード。 - 1質問あたり1警告に絞るためのDB制約や
with_lockの活用、もしくは何らかの冪等キーでのUpsert。
リポジトリ全体での該当箇所を洗い出す補助スクリプトです（実際のリファクタは別PRで十分です）:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "[scan] QuestionAutoCloser の候補ボトルネック（answers.select/any? など）"
fd -a question_auto_closer.rb | xargs -I{} rg -n -C2 -e 'find_each' -e '\.answers\.select' -e '\.answers\.any\?' "{}" || trueLength of output: 974
要対応: QuestionAutoCloser の N+1 と同時実行による重複生成リスク（確認済み））
リポジトリ内検索の結果、QuestionAutoCloser 内で find_each と Ruby 側フィルタ（answers.select / answers.any?）が確認されました。大量データで N+1 やメモリ負荷、かつ「判定 → 作成」が非原子的なため同時実行時に警告コメントが重複して生成されるリスクがあります。
該当箇所（確認済み）
- app/controllers/scheduler/daily/auto_close_questions_controller.rb — lines 5–6:
QuestionAutoCloser.post_warning/QuestionAutoCloser.close_and_select_best_answer - question_auto_closer.rb —
find_eachのループ（lines 16, 27） - question_auto_closer.rb —
question.answers.select（line 60） - question_auto_closer.rb —
question.answers.any?（line 67）
推奨対応（優先順）
- 判定ロジックを可能な限りSQL側へ移行する（
answers.where(...).exists?/joinsやサブクエリ／EXISTS を利用）。LIKE/正規表現はDB依存なので注意。 - N+1対策として
includes(:answers)や必要カラムのみを事前ロードする（または join + distinct で絞る）。 - 重複防止は DB レベルのユニーク制約 ＋
create_or_find_by/upsert、あるいはwith_lock／トランザクション／アドバイザリロックで原子的に処理する。 - バッチ処理・メモリ対策：
find_eachのバッチサイズ調整や不要オブジェクト生成の抑制。
具体的なリファクタは別PRで問題ありませんが、上記を踏まえて優先的に対応してください。
There was a problem hiding this comment.
大量データで N+1 やメモリ負荷
過去の運用実績からすると、未解決の質問（Question.not_solved）は5件以下、一つの質問に対する回答は平均数件で多くても数十件なので、メモリ負荷はあまり意識しなくて大丈夫です。
ただし、指摘の通り現状のコードは非効率なのでN+1対策とロジックのSQL側への移行は別PRで検討します。
「判定 → 作成」が非原子的なため同時実行時に警告コメントが重複して生成されるリスクがあります
app/controllers/scheduler/daily/auto_close_questions_controller.rbのメソッドはdailyで自動的に一度だけ呼び出されるため同時実行リスクはありません。
There was a problem hiding this comment.
@ryufuta, 運用実績に基づく詳細な情報をありがとうございます。
了解しました:
- 未解決質問が5件以下、回答も数件〜数十件程度という実績であれば、現状の実装でメモリ負荷は問題ないですね
- N+1対策とSQL側への移行は別PRで検討いただけるとのこと、承知しました
- dailyで1回のみ自動実行される仕様であれば、同時実行による重複リスクは確かに存在しませんね
本PRはバグ修正が目的ですので、パフォーマンス最適化は別PRで対応するアプローチが適切だと思います。貴重なコンテキストをご共有いただき、ありがとうございました！
🧠 Learnings used
Learnt from: ryufuta
Repo: fjordllc/bootcamp PR: 9323
File: app/models/question_auto_closer.rb:45-51
Timestamp: 2025-12-03T06:39:24.048Z
Learning: fjordllc/bootcamp の QuestionAutoCloser において、create_warning_message で発行される 'answer.create' イベントによってAnswererWatcherがシステムユーザーを質問の監視者として追加する動作は、プロダクトオーナーの判断により問題ないとされており、現状のまま維持される。
Learnt from: ryufuta
Repo: fjordllc/bootcamp PR: 9323
File: app/models/question_auto_closer.rb:45-51
Timestamp: 2025-11-25T05:42:10.478Z
Learning: fjordllc/bootcamp の QuestionAutoCloser において、create_warning_message で発行される 'answer.create' イベントは、システムユーザーによる自動クローズ警告メッセージを質問者と既存の監視者に通知するための意図的な設計である。AnswerNotifier と NotifierToWatchingUser による通知は期待される動作。
Learnt from: jun-kondo
Repo: fjordllc/bootcamp PR: 9202
File: config/initializers/active_support_notifications.rb:6-7
Timestamp: 2025-11-03T04:53:33.424Z
Learning: app/controllers/api/answers_controller.rb の destroy アクションにおいて、answer.destroy イベントは `if answer.type == 'CorrectAnswer'` 条件付きで発行される。通常の Answer 削除では未解決問題数に影響しないため、ベストアンサー (CorrectAnswer) 削除時のみ AnswerCacheDestroyer によるキャッシュクリアが必要。
Learnt from: komagata
Repo: fjordllc/bootcamp PR: 9101
File: app/models/graduation_notifier.rb:23-28
Timestamp: 2025-09-12T21:16:47.639Z
Learning: Rails upgrade PRにおいて、configuration systemの変更やwebhook設定の改善提案も、アップグレードに直接関連しない場合は別PRで対応すべきである。PRのスコープ維持が重要。
Learnt from: matuaya
Repo: fjordllc/bootcamp PR: 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）必ず実行する必要がある。
Learnt from: ryufuta
Repo: fjordllc/bootcamp PR: 9155
File: app/controllers/api/correct_answers_controller.rb:20-22
Timestamp: 2025-09-15T07:39:39.350Z
Learning: API::CorrectAnswersController#update では、ベストアンサー取り下げという単純な状態変更のため、update! を使用して DB エラー時に 500 を返し、成功時はデフォルトレスポンスを使用するのが適切。クライアント側のバリデーションエラーは発生しにくく、明示的なエラーハンドリングは不要。
Learnt from: e-yanagita-gs
Repo: fjordllc/bootcamp PR: 8771
File: app/controllers/welcome_controller.rb:57-57
Timestamp: 2025-06-30T01:07:46.192Z
Learning: プロジェクトでは、仕様上データの存在が保証されている場合、nil処理を省略することがある。特にFAQ表示のような特定機能に特化したPRでは、データの存在を前提とした実装が採用される。
Learnt from: sjabcdefin
Repo: fjordllc/bootcamp PR: 9247
File: app/controllers/checks_controller.rb:0-0
Timestamp: 2025-10-22T06:04:36.036Z
Learning: ChecksController#createおよび#destroyでは、Checkの作成・削除とActiveSupport::Notifications.instrumentによるイベント発行(プラクティスのステータス更新)を同一トランザクション内で実行し、いずれかが失敗した場合は両方をロールバックする。これによりWebUI表示とDB状態の整合性を保証している。
Learnt from: Miya096jp
Repo: fjordllc/bootcamp PR: 9102
File: db/fixtures/buzzes.yml:0-0
Timestamp: 2025-09-04T01:27:23.422Z
Learning: The buzz_controller.rb was indeed deleted, but there are still two remaining items from the old buzz single page implementation that need cleanup: the data migration file db/data/20230726024528_create_buzz.rb and the old buzz route in config/routes.rb line 136. These should be updated/removed to complete the buzz page redesign migration.
Learnt from: Miya096jp
Repo: fjordllc/bootcamp PR: 9102
File: db/fixtures/buzzes.yml:0-0
Timestamp: 2025-09-04T01:27:23.422Z
Learning: The buzz_controller.rb and related data migration files were deleted as part of the Buzz page redesign, not just modified. The old single buzz page was completely replaced with a new buzzes (plural) listing page with year-based navigation and metadata structure.
Learnt from: ryufuta
Repo: fjordllc/bootcamp PR: 8992
File: app/models/correct_answer_notifier.rb:23-23
Timestamp: 2025-07-30T07:26:36.599Z
Learning: PRのスコープを維持することが重要であり、バグ修正PRにおいて、修正対象以外の機能（例：ルーティングテスト）の追加提案は適切ではない。そのような改善が必要な場合は別PRで対応すべきである。
Learnt from: komagata
Repo: fjordllc/bootcamp PR: 9101
File: app/models/product.rb:59-61
Timestamp: 2025-09-11T14:47:53.256Z
Learning: Rails アップグレードPRでは、アップグレードに直接関連しない性能改善や機能追加の提案は避けるべき。PRのスコープを維持することが重要で、そのような改善提案は別PRで対応すべきである。
Learnt from: komagata
Repo: fjordllc/bootcamp PR: 9101
File: app/notifiers/discord_notifier.rb:131-135
Timestamp: 2025-09-12T21:18:00.834Z
Learning: Rails アップグレードPRにおいて、product_review_not_completed メソッドの webhook URL 設定の shared フォールバック追加も、設定システム全体の変更として別PRで対応すべきである。
Learnt from: komagata
Repo: fjordllc/bootcamp PR: 9101
File: app/notifiers/discord_notifier.rb:52-56
Timestamp: 2025-09-12T21:17:45.295Z
Learning: Rails upgrade PRにおいて、configuration systemの改善提案（shared configuration fallback等）も、アップグレードに直接関連しない場合は別PRで対応すべきである。PRのスコープ維持が重要。
|
@komagata coderabbitaiの指摘 #9024 (comment) についてはリファクタの別PRで問題ないとあり、descriptionに記載した通り当初からその予定でしたので未対応です。 |
|
レビュアーに判断を丸投げする形の書き方になってしまい申し訳ありません🙇🏻♂️
1と3について。 2について。 |
CodeRabbitの指摘への対応: #9024 (comment)
CodeRabbitの指摘への対応: #9024 (comment)
CodeRabbitの指摘への対応: #9024 (comment)
CodeRabbitの指摘への対応: #9024 (comment)
CodeRabbitの指摘への対応: #9024 (comment)
CodeRabbitの指摘への対応: #9024 (comment)
2 participants
Issue
概要
変更確認方法
以下の動作が正しく行われるようになったことを確認する。
事前準備
bug/question-not-auto-closeをローカルに取り込むbin/rails db:seedを実行して初期データを更新するTOKENを設定するローカル環境に環境変数を設定するには
dotenv-railsというgemを使用すると便利。以下はこのgemを使用する場合の手順。
（
Gemfileに追記済みのため改めてインストールする必要はない）.env.localファイルを作成TOKEN=hogeを追記動作確認
foreman start -f Procfile.devでローカルサーバーを起動Screenshot
変更前
http://localhost:3000/scheduler/daily/auto_close_questions?token=hoge にアクセスすると
HTTP ERROR 500となって定期処理が実行されない。変更後
懸念点
以下の懸念点があるが、このPRの範囲外と判断して対処していない。
必要であれば別のPRで対処したい。
ベストアンサーの登録に潜在的なバグがある
1つのQ&Aに対してベストアンサーは0または1つの想定だが、現状ではモデルにもDBにもユニーク制約がないため2つ以上のベストアンサーが登録され得る。
2つ以上のベストアンサーが登録された場合は最新のベストアンサーのみが画面上に表示され、古いベストアンサーはDB上には存在するが画面上には回答自体が表示されないことをローカル環境で確認済み。
通常の操作においても、複数ユーザーが同時に異なる回答をベストアンサーに選んだ場合や自動クローズの実行中にユーザーがベストアンサーを選んだ場合にこのバグが発生する可能性がある。
自動クローズの解除方法の仕様が不明確
仕様が規定されていないが、現状では以下の仕様になっている。
#8086 を見ると、もともとはGitHubのissueの自動クローズのようなものを想定しているようなので、そちらと似た仕様にしても良いかもしれない。
QuestionAutoCloserクラスのリファクタリング
無駄な処理（
close_with_best_answer内でクロージングコメントを保存してすぐ削除している箇所など）を変更する、インスタンス化して使うようにするなど、リファクタリングの余地が大きいと感じた。Summary by CodeRabbit