Conversation
## Walkthrough
`markdown_to_plain_text` メソッドが `md2plain_text` にリネームされ、これに伴い `SearchHelper` 内の呼び出しも更新されました。`Commentable` concern の `url` メソッドで `Talk`、`RegularEvent`、`Movie` クラスが新たにサポートされました。また、環境判定の記述が簡潔化されました。テストのタグ追加検証方法も変更されています。
## Changes
| ファイル/グループ | 変更内容概要 |
|----------------------------------------------------|---------------------------------------------------------------------|
| app/helpers/markdown_helper.rb | `markdown_to_plain_text` を `md2plain_text` にリネーム。 |
| app/helpers/search_helper.rb | `markdown_to_plain_text` の呼び出しを `md2plain_text` に変更。 |
| app/models/concerns/commentable.rb | `url` メソッドで `Talk`、`RegularEvent`、`Movie` クラスを追加サポート。|
| app/helpers/page_tabs/users_helper.rb | Rails環境判定を `Rails.env.in? %w[development test]` に簡素化。 |
| test/system/article/tags_test.rb | タグ追加の検証を `.tagify__tag` 要素から `input[name="article[tag_list]"]` の値に変更。|
## Suggested reviewers
- komagata
## 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.75.5)test/helpers/markdown_helper_test.rbrubocop-minitest extension supports plugin, specify 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🧰 Additional context used📓 Path-based instructions (2)**/*.rbInstructions used from: Sources: test/**/*Instructions used from: Sources: 🧠 Learnings (2)📓 Common learningstest/helpers/markdown_helper_test.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 (1)
✨ Finishing Touches
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
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: 4
🧹 Nitpick comments (12)
app/models/report.rb (1)
14-14: SearchHelper を全モデルで個別にincludeするのは重複が多いですすでに
Searchableを mix-in しているので、Searchable内部でinclude SearchHelperしてしまえば各モデルに同じ1行を追加する必要がなく DRY になります。
今回のように手動で足して回る運用は将来の漏れ・差分混入を誘発しやすいので、一度ご検討ください。app/models/regular_event.rb (1)
32-32: RegularEvent も統一的に検索対象へこれで検索対象モデルの一貫性が取れました。テストで RegularEvent が検索結果に含まれるかも合わせて確認してください。
app/models/concerns/commentable.rb (1)
31-44: より簡潔で保守性の高い実装への改善を提案します。現在の実装は機能的には正しいですが、Railsの慣習に従ってより簡潔に書き直せます。
以下の改善を提案します：
- def url - case self - when Product - Rails.application.routes.url_helpers.product_path(self) - when Announcement - Rails.application.routes.url_helpers.announcement_path(self) - when Event - Rails.application.routes.url_helpers.event_path(self) - when Report - Rails.application.routes.url_helpers.report_path(self) - else - raise NotImplementedError, "#{self.class} does not have a defined URL path" - end - end + def url + Rails.application.routes.url_helpers.polymorphic_path(self) + rescue NoMethodError + raise NotImplementedError, "#{self.class} does not have a defined URL path" + endこの実装により、コードがより簡潔になり、将来的に新しいcommentableタイプが追加された場合も自動的に対応できます。
app/models/search_user.rb (1)
15-15: 行が長すぎて可読性が低下しています。この行は複雑すぎて理解しにくいです。可読性を向上させるためにリファクタリングを検討してください。
- searched_user = @users ? @users.merge(User.search_by_keywords(word: validated_search_word, exact_match: true)) : User.search_by_keywords(word: validated_search_word, exact_match: true) + if @users + searched_user = @users.merge(User.search_by_keywords(word: validated_search_word, exact_match: true)) + else + searched_user = User.search_by_keywords(word: validated_search_word, exact_match: true) + endtest/helpers/search_hepler_test.rb (1)
1-70: ファイル名にタイポがあります。ファイル名が
search_hepler_test.rbとなっていますが、正しくはsearch_helper_test.rbであるべきです。テストの実装自体は適切で、SearchHelperモジュールの各メソッドが適切にテストされています。
app/models/search_result.rb (1)
6-8: 多くの属性を持つクラスです。このクラスは多数の属性を持っており、単一責任の原則に違反している可能性があります。将来的にはより小さなクラスに分割することを検討してください。
app/views/searchables/index.html.slim (1)
16-16:talkモデルの除外ロジックについて説明が必要です。なぜ
talkモデルを明示的にスキップしているのか、コメントを追加することを推奨します。- - next if searchable.model_name == 'talk' + - next if searchable.model_name == 'talk' # Talk は検索結果に表示しない仕様app/helpers/search_helper.rb (1)
14-14: 複雑な条件式をリファクタリングして可読性を向上させてください。条件式が複雑で理解しにくいため、メソッドに抽出することを推奨します。
- return process_special_case(comment, word) if comment.is_a?(String) && comment.include?('|') && !comment.include?('```') + return process_special_case(comment, word) if special_case_string?(comment) + private + + def special_case_string?(comment) + comment.is_a?(String) && comment.include?('|') && !comment.include?('```') + endapp/models/concerns/searchable.rb (1)
65-77: ラベル生成ロジックの一貫性を改善できます。一部のラベルは I18n を使用し、他はハードコーディングされています。可能であれば、すべてのラベルを I18n に統一することを検討してください。
case model_name.name.downcase - when 'regularevent' then "定期\nイベント" - when 'event' then "特別\nイベント" - when 'practice' then "プラク\nティス" + when 'regularevent' then I18n.t('searchable.labels.regular_event', default: "定期\nイベント") + when 'event' then I18n.t('searchable.labels.event', default: "特別\nイベント") + when 'practice' then I18n.t('searchable.labels.practice', default: "プラク\nティス")app/models/searcher.rb (3)
73-75: フィルタリング処理を最適化できます。
searchablesが nil の場合は空配列を返すようにして、不要な処理を避けることができます。def filter_results!(searchables, current_user) - searchables&.select { |searchable| visible_to_user?(searchable, current_user) } + return [] if searchables.nil? + searchables.select { |searchable| visible_to_user?(searchable, current_user) } end
124-127: 検索クエリの可読性を改善できます。Arel を使用してより読みやすく保守しやすいコードにすることができます。
def search_users(words) - User.where(words.map { |_word| 'login_name ILIKE ? OR name ILIKE ? OR description ILIKE ?' } - .join(' AND '), *words.flat_map { |word| ["%#{word}%"] * 3 }) + scope = User.all + words.each do |word| + pattern = "%#{word}%" + scope = scope.where( + "login_name ILIKE :pattern OR name ILIKE :pattern OR description ILIKE :pattern", + pattern: pattern + ) + end + scope end
144-152: 早期リターンでコードの可読性を向上できます。ネストを減らして、より読みやすいコードにできます。
def extract_user_id_match(result, word) user_id = word.delete_prefix('user:') - return result.user&.login_name&.casecmp?(user_id) if result.respond_to?(:user) && result.user.present? - if result.respond_to?(:last_updated_user_id) && result.last_updated_user_id.present? - return User.find_by(id: result.last_updated_user_id)&.login_name&.casecmp?(user_id) - end - - false + + if result.respond_to?(:user) && result.user.present? + return result.user.login_name&.casecmp?(user_id) + end + + if result.respond_to?(:last_updated_user_id) && result.last_updated_user_id.present? + return User.find_by(id: result.last_updated_user_id)&.login_name&.casecmp?(user_id) + end + + false end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (39)
app/controllers/api/searchables_controller.rb(0 hunks)app/controllers/searchables_controller.rb(1 hunks)app/helpers/markdown_helper.rb(4 hunks)app/helpers/page_tabs/users_helper.rb(1 hunks)app/helpers/search_helper.rb(3 hunks)app/javascript/components/Searchable.jsx(0 hunks)app/javascript/components/Searchables.jsx(0 hunks)app/models/announcement.rb(1 hunks)app/models/answer.rb(1 hunks)app/models/comment.rb(2 hunks)app/models/concerns/commentable.rb(1 hunks)app/models/concerns/searchable.rb(1 hunks)app/models/correct_answer.rb(1 hunks)app/models/event.rb(1 hunks)app/models/page.rb(2 hunks)app/models/practice.rb(1 hunks)app/models/product.rb(1 hunks)app/models/question.rb(1 hunks)app/models/regular_event.rb(1 hunks)app/models/report.rb(1 hunks)app/models/search_result.rb(1 hunks)app/models/search_user.rb(2 hunks)app/models/searcher.rb(1 hunks)app/models/talk.rb(1 hunks)app/models/user.rb(1 hunks)app/views/activity_mailer/assigned_as_checker.html.slim(1 hunks)app/views/api/searchables/_searchable.json.jbuilder(1 hunks)app/views/searchables/index.html.slim(1 hunks)app/views/watches/_watch.html.slim(1 hunks)config/routes/api.rb(0 hunks)db/fixtures/comments.yml(2 hunks)test/fixtures/comments.yml(3 hunks)test/helpers/search_hepler_test.rb(1 hunks)test/integration/api/searchables_test.rb(0 hunks)test/models/comment_test.rb(1 hunks)test/models/searcher_test.rb(1 hunks)test/models/user_test.rb(1 hunks)test/system/current_user/bookmarks_test.rb(1 hunks)test/system/searchables_test.rb(1 hunks)
💤 Files with no reviewable changes (5)
- config/routes/api.rb
- app/controllers/api/searchables_controller.rb
- app/javascript/components/Searchables.jsx
- test/integration/api/searchables_test.rb
- app/javascript/components/Searchable.jsx
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: hirokiej
PR: fjordllc/bootcamp#8740
File: app/helpers/reports_helper.rb:56-64
Timestamp: 2025-06-29T03:44:15.162Z
Learning: このプロジェクト（fjordllc/bootcamp）では、ja.ymlファイルで一部の単語や文章のみI18n対応されているが、多くのテキストは日本語でハードコーディングされており、完全な国際化対応は行われていない。新しい機能でもI18n対応は不要と判断される。
app/models/page.rb (1)
Learnt from: e-yanagita-gs
PR: fjordllc/bootcamp#8771
File: app/controllers/welcome_controller.rb:57-57
Timestamp: 2025-06-30T01:07:46.170Z
Learning: プロジェクトでは、仕様上データの存在が保証されている場合、nil処理を省略することがある。特にFAQ表示のような特定機能に特化したPRでは、データの存在を前提とした実装が採用される。
app/views/activity_mailer/assigned_as_checker.html.slim (2)
Learnt from: tyrrell-IH
PR: fjordllc/bootcamp#8807
File: app/views/welcome/job_support.html.slim:391-392
Timestamp: 2025-06-21T22:30:20.116Z
Learning: このプロジェクトにはRailsの組み込みmarkdownヘルパが利用できないため、markdownのHTML変換には自前でヘルパを実装する必要がある。
Learnt from: mousu-a
PR: fjordllc/bootcamp#8566
File: app/views/activity_mailer/matching_pair_work.html.slim:6-9
Timestamp: 2025-06-30T03:27:35.940Z
Learning: このプロジェクトのmd2htmlヘルパーにはサニタイゼーション処理がなく、ユーザー入力をHTMLに変換する際のXSS脆弱性が存在する。この問題は複数のメールViewで共通して発生している。
test/fixtures/comments.yml (1)
Learnt from: tyrrell-IH
PR: fjordllc/bootcamp#8807
File: app/views/welcome/job_support.html.slim:391-392
Timestamp: 2025-06-21T22:30:20.116Z
Learning: このプロジェクトにはRailsの組み込みmarkdownヘルパが利用できないため、markdownのHTML変換には自前でヘルパを実装する必要がある。
app/helpers/markdown_helper.rb (2)
Learnt from: tyrrell-IH
PR: fjordllc/bootcamp#8807
File: app/views/welcome/job_support.html.slim:391-392
Timestamp: 2025-06-21T22:30:20.116Z
Learning: このプロジェクトにはRailsの組み込みmarkdownヘルパが利用できないため、markdownのHTML変換には自前でヘルパを実装する必要がある。
Learnt from: mousu-a
PR: fjordllc/bootcamp#8566
File: app/views/activity_mailer/matching_pair_work.html.slim:6-9
Timestamp: 2025-06-30T03:27:35.940Z
Learning: このプロジェクトのmd2htmlヘルパーにはサニタイゼーション処理がなく、ユーザー入力をHTMLに変換する際のXSS脆弱性が存在する。この問題は複数のメールViewで共通して発生している。
test/system/current_user/bookmarks_test.rb (2)
Learnt from: hirokiej
PR: fjordllc/bootcamp#8740
File: app/helpers/reports_helper.rb:56-64
Timestamp: 2025-06-29T03:44:15.162Z
Learning: このプロジェクト（fjordllc/bootcamp）では、ja.ymlファイルで一部の単語や文章のみI18n対応されているが、多くのテキストは日本語でハードコーディングされており、完全な国際化対応は行われていない。新しい機能でもI18n対応は不要と判断される。
Learnt from: tyrrell-IH
PR: fjordllc/bootcamp#8807
File: app/views/welcome/job_support.html.slim:391-392
Timestamp: 2025-06-21T22:30:20.116Z
Learning: このプロジェクトにはRailsの組み込みmarkdownヘルパが利用できないため、markdownのHTML変換には自前でヘルパを実装する必要がある。
app/helpers/search_helper.rb (1)
Learnt from: tyrrell-IH
PR: fjordllc/bootcamp#8807
File: app/views/welcome/job_support.html.slim:391-392
Timestamp: 2025-06-21T22:30:20.116Z
Learning: このプロジェクトにはRailsの組み込みmarkdownヘルパが利用できないため、markdownのHTML変換には自前でヘルパを実装する必要がある。
🧬 Code Graph Analysis (12)
app/models/user.rb (8)
app/models/page.rb (1)
include(3-44)app/models/announcement.rb (1)
include(3-40)app/models/answer.rb (1)
include(3-31)app/models/comment.rb (1)
include(3-69)app/models/practice.rb (1)
include(3-232)app/models/product.rb (1)
include(3-228)app/models/question.rb (1)
include(3-115)app/models/report.rb (1)
include(3-153)
test/models/user_test.rb (1)
app/models/search_user.rb (1)
search(11-24)
app/models/page.rb (1)
app/models/concerns/commentable.rb (1)
body(22-29)
app/models/comment.rb (4)
test/models/comment_test.rb (1)
include(5-116)app/models/answer.rb (1)
include(3-31)app/models/talk.rb (1)
include(3-13)app/models/concerns/commentable.rb (1)
title(13-20)
app/helpers/page_tabs/users_helper.rb (2)
app/controllers/application_controller.rb (1)
test?(69-71)app/models/user.rb (1)
latest_micro_report_page(920-922)
app/helpers/markdown_helper.rb (2)
app/models/search_result.rb (1)
include(3-60)test/helpers/application_helper_test.rb (1)
include(5-41)
app/controllers/searchables_controller.rb (1)
app/models/searcher.rb (1)
search(16-24)
test/models/comment_test.rb (1)
app/models/comment.rb (1)
title(60-62)
app/models/search_user.rb (1)
app/models/concerns/searchable.rb (1)
search_by_keywords(14-16)
app/models/search_result.rb (3)
app/helpers/search_helper.rb (4)
searchable_url(31-41)searchable_summary(9-19)filtered_message(43-53)created_user(55-61)app/models/concerns/searchable.rb (2)
formatted_updated_at(58-63)primary_role(51-56)app/models/user.rb (1)
avatar_url(706-718)
test/helpers/search_hepler_test.rb (1)
app/helpers/search_helper.rb (3)
searchable_url(31-41)filtered_message(43-53)created_user(55-61)
app/models/searcher.rb (10)
app/models/page.rb (1)
include(3-44)app/models/search_result.rb (1)
include(3-60)app/models/announcement.rb (1)
include(3-40)app/models/answer.rb (1)
include(3-31)app/models/comment.rb (1)
include(3-69)app/models/event.rb (1)
include(3-167)app/models/practice.rb (1)
include(3-232)app/models/product.rb (1)
include(3-228)app/models/question.rb (1)
include(3-115)app/models/user.rb (1)
include(3-956)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build_and_test
🔇 Additional comments (45)
app/helpers/markdown_helper.rb (4)
4-4: OutputSafetyHelperの追加による安全性向上セキュリティ向上のために適切なヘルパーモジュールが追加されています。これにより、HTMLの安全な処理が可能になります。
20-20: strip_tagsからfull_sanitizerへの変更によるセキュリティ強化
strip_tagsからActionView::Base.full_sanitizer.sanitizeへの変更は、以前指摘されていたXSS脆弱性への対応として適切です。Railsの標準的なサニタイザーを使用することで、より安全なHTML処理が実現されています。
43-43: 一貫したサニタイゼーション処理
process_markdown_caseメソッドでも同様にfull_sanitizerを使用しており、アプリケーション全体で一貫したサニタイゼーション処理が実現されています。
61-66: 新しいmarkdown_to_plain_textメソッドの追加Markdownコンテンツをプレーンテキストに変換する新しいメソッドが追加されています。実装は適切で、空白チェック、HTML変換、サニタイゼーション、空白文字の除去という適切な処理フローになっています。検索機能の強化において、統一されたMarkdown処理を提供する重要な役割を果たしています。
app/models/answer.rb (1)
7-7: SearchHelperモジュールの追加検索機能の統一化に向けて、AnswerモデルにSearchHelperモジュールが追加されています。他のモデルと一貫性があり、検索結果の統一された処理を実現する適切な変更です。
app/models/question.rb (1)
11-11: SearchHelperモジュールの追加検索機能の統一化に向けて、QuestionモデルにSearchHelperモジュールが追加されています。他のモデルと一貫性があり、検索結果の統一された処理を実現する適切な変更です。
app/models/practice.rb (1)
6-6: SearchHelperモジュールの追加検索機能の統一化に向けて、PracticeモデルにSearchHelperモジュールが追加されています。他のモデルと一貫性があり、検索結果の統一された処理を実現する適切な変更です。
app/models/announcement.rb (1)
10-10: SearchHelperモジュールの追加検索機能の統一化に向けて、AnnouncementモデルにSearchHelperモジュールが追加されています。他のモデルと一貫性があり、検索結果の統一された処理を実現する適切な変更です。
app/models/talk.rb (1)
6-6:include順序は意図どおり？
SearchHelperをSearchableの後で mix-in しているため、同名メソッドがあればSearchHelper側が優先されます。
衝突を避けたい場合はモジュール側でprependする・名前を変える等の方針を明示しておくと安心です。app/models/event.rb (1)
10-10: イベントでもSearchHelperを導入したことで URL 生成などの helper が利用可能になりました現状の実装を見る限り Event には同名メソッドが存在しないため衝突はありません。
特に問題ないので、この変更は歓迎です。app/models/user.rb (1)
7-7: User にも検索 helper が入りましたが性能インパクトを確認してくださいUser 検索は件数が多いテーブルでボトルネックになりがちです。
SearchHelperが Markdown レンダリングやポリシーチェックを行う場合、N+1 や重い文字列操作が呼ばれないか念のためベンチを取ることを推奨します。app/models/correct_answer.rb (1)
6-6: SearchHelper の追加が適切です他のモデルと同様に SearchHelper を include することで、検索機能の統一が図られています。変更内容は一貫性があり、問題ありません。
app/models/product.rb (1)
16-16: SearchHelper の追加が適切ですProduct モデルにも SearchHelper を include することで、検索機能の統一が図られています。他のモデルと同様の変更で一貫性があります。
app/views/watches/_watch.html.slim (1)
32-32: body 属性への変更が適切です
descriptionからbodyへの変更により、検索システムのリファクタリングと一貫性が保たれています。他の部分でも同様の変更が行われており、統一されたアプローチです。app/models/page.rb (2)
11-11: SearchHelper の追加が適切です他のモデルと同様に SearchHelper を include することで、検索機能の統一が図られています。
33-35: description メソッドの追加が適切です
bodyを返すdescriptionメソッドにより、検索結果での一貫したインターフェースが提供されています。コメントも分かりやすく、実装の意図が明確です。app/models/comment.rb (2)
7-7: SearchHelper の追加が適切です他のモデルと同様に SearchHelper を include することで、検索機能の統一が図られています。
60-62: title メソッドの実装が適切ですコメントの親オブジェクトから title を取得する仕組みにより、検索結果での表示が改善されています。
respond_to?を使った安全な実装で、エラーを防いでいます。app/views/activity_mailer/assigned_as_checker.html.slim (1)
4-4: 変更は適切ですが、既知のセキュリティ問題に注意
@product.descriptionから@product.bodyへの変更は、アプリケーション全体でのmarkdownコンテンツ処理の統一化と一致しており適切です。ただし、学習履歴によると
md2htmlヘルパーにはサニタイゼーション処理がなく、XSS脆弱性が存在することが指摘されています。メールテンプレートでは直接的なXSSリスクは低いものの、将来的な改善が必要です。db/fixtures/comments.yml (2)
7-7: テストデータの整理として適切CSSの周りのmarkdown太字フォーマットの削除は、更新されたmarkdown処理ロジックとの整合性を保つため適切です。
45-45: 絵文字表記の統一として適切絵文字ショートコード
:sweat_drops:からUnicode絵文字💦への変更は、絵文字レンダリングロジックの統一化と一致しており適切です。test/models/user_test.rb (1)
333-335: 新しい検索インターフェースに適切に対応
Searcher.searchにcurrent_user: hajimeパラメータを渡し、結果をSearchResultオブジェクトからUserオブジェクトにマッピングする変更は、リファクタリングされた検索機能と正しく連携しています。新しい検索アーキテクチャでは
current_userコンテキストが必要であり、この更新により適切にテストが機能します。test/system/current_user/bookmarks_test.rb (1)
33-33: テキスト表示の精密なマッチングのため適切日本語の句点「。」の後にスペースを追加する変更は、サーバーサイドレンダリングに移行した検索結果表示での正確なテキストマッチングを確保するため適切です。
test/system/searchables_test.rb (1)
14-14: サーバーサイドレンダリングに適応した適切な更新CSSセレクターを
.card-list-item__summaryから.card-list-item__summary.pに変更することで、paragraph要素に限定してより具体的な要素選択を行っています。これは、ReactコンポーネントからサーバーレンダリングされたHTMLへの移行に伴うHTML構造の変化に正しく対応した更新です。
app/views/api/searchables/_searchable.json.jbuilder (1)
1-1: SearchHelperモジュールへの統一化が適切に実装されています。検索関連のロジックがSearchHelperモジュールに集約されており、コードの一貫性と保守性が向上しています。
test/models/comment_test.rb (1)
112-115: 新しいtitleメソッドのテストケースが適切に実装されています。Comment#titleメソッドの動作を検証するテストケースが追加されており、fixture dataを使用した適切なテストになっています。
test/fixtures/comments.yml (3)
7-7: マークダウン記法の削除により、プレーンテキストでのテストが可能になりました。検索機能のテストにおいて、マークダウン記法が含まれないテストケースが追加されることで、より包括的なテストが可能になります。
45-47: Unicode絵文字の使用とフォーマットの改善が適切です。絵文字ショートコードからUnicode絵文字への変更により、より現実的なテストデータになっています。
180-184: 新しいテストフィクスチャの追加により、テストカバレッジが向上しています。product9に対するコメントフィクスチャが追加されており、検索機能のテストに必要なデータが適切に提供されています。
app/helpers/page_tabs/users_helper.rb (2)
21-21: 相談部屋タブの権限管理が適切に実装されています。管理者のみが非管理者ユーザーの相談部屋を表示できるという権限管理が適切に実装されており、セキュリティ上の配慮も十分です。
16-20: 分報タブの環境制限について理由の明示をお願いします現状、app/helpers/page_tabs/users_helper.rb（16–20行目）で分報タブの表示が
Rails.env.development? || Rails.env.test?に限定されていますが、プロダクション環境で非表示とする意図がコード上やドキュメント上に記載されていないため、その背景（例：機能がまだ安定していない、社内用機能のため公開しない、など）をコメントもしくはREADMEに明示してください。app/controllers/searchables_controller.rb (2)
4-4: 定数の定義が適切です。ページサイズを定数として定義することで、メンテナンスしやすくなっています。
6-12: 検索機能の実装が適切です。パラメータの抽出、Searcherサービスの呼び出し、ページネーションの実装がすべて適切に行われています。current_userパラメータの追加により、ユーザー固有の検索結果フィルタリングが可能になっています。
app/models/search_user.rb (2)
13-13: 早期リターンの実装が適切です。空白の検索語に対して早期リターンを行うことで、不要な処理を避けています。
27-37: 検索語の検証ロジックが改善されています。nil と空文字列の区別が明確になり、検証ロジックが分かりやすくなっています。
test/models/searcher_test.rb (2)
19-27: テストヘルパーメソッドの追加が適切です。
current_userメソッドとstrip_htmlメソッドの追加により、テストコードが統一され、可読性が向上しています。
28-49: 新しい検索機能のテストが適切に実装されています。ユーザーフィルタリングと可視性に基づく検索結果の検証が適切に行われています。current_userパラメータの必須化に対応したテストの更新も適切です。
app/views/searchables/index.html.slim (2)
76-81: 空の検索結果表示の実装が良好です。ユーザーフレンドリーなメッセージとアイコンで、検索結果が見つからない場合の表示が適切に実装されています。
1-8: 検索ワードのHTMLエスケープは適切に行われています
検証の結果、Slim テンプレートの=出力（h2.page-header__title = "#{@word} の検索結果"）は自動的に HTML エスケープされるため、XSS の懸念はありません。
- app/views/searchables/index.html.slim: h2.page-header__title = "#{@word} の検索結果"
app/helpers/search_helper.rb (2)
32-41: URL 生成ロジックの改善が素晴らしいです。
case文を使用したリファクタリングにより、コードの可読性と保守性が向上しています。Rails の route helpers を適切に活用しています。
43-53: 権限チェックと Markdown 処理の実装が適切です。Policy パターンを使用した権限チェックと、一貫した Markdown 処理により、セキュアで保守しやすいコードになっています。
app/models/concerns/searchable.rb (2)
10-10: ActiveRecord 以外のクラスへの対応が適切です。
ActiveRecord::Baseのチェックにより、このモジュールがより柔軟に使用できるようになっています。
58-63: 日付フォーマットの実装が適切です。日本語の曜日表示を含む、ユーザーフレンドリーな日付フォーマットが実装されています。
app/models/searcher.rb (2)
16-24: ユーザーコンテキストを考慮した検索実装が優れています。
current_userパラメータの追加により、権限に基づいた適切な検索結果のフィルタリングが可能になっています。
137-142: キーワードマッチングの実装が堅牢です。
tryメソッドを使用した nil-safe な実装により、様々なモデルタイプに対応できています。
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/views/searchables/index.html.slim (1)
72-75: N+1クエリの問題が残っています。以前のレビューコメントで指摘された問題が修正されていません。
Talk.find_byが検索結果ループ内で実行されるため、N+1クエリが発生します。コントローラーで事前にロードした
@talks_by_user_idを使用するように修正してください：- - talk = Talk.find_by(user_id: searchable_user.id) + - talk = @talks_by_user_id[searchable_user.id]
🧹 Nitpick comments (4)
test/helpers/search_hepler_test.rb (2)
59-63: 変数名の重複により混乱を招いています。
created_userメソッドをテストしていますが、同じ名前の変数を使用しているため、可読性が低下しています。次の修正を適用してください：
test 'created_user returns the correct user for SearchResult' do searchable_result = SearchResult.new(@report, 'test', @user_komagata) - created_user = created_user(searchable_result) - assert_equal @user_komagata, created_user + result_user = created_user(searchable_result) + assert_equal @user_komagata, result_user end
65-68: 変数名の重複により混乱を招いています。同様に、このテストでも変数名の重複があります。
次の修正を適用してください：
test 'created_user returns the correct user for Comment' do - created_user = created_user(@comment) - assert_equal @comment.user, created_user + result_user = created_user(@comment) + assert_equal @comment.user, result_user endapp/controllers/searchables_controller.rb (1)
4-4: 定数の命名に一貫性がありません。Railsの慣習では、このような定数は
ITEMS_PER_PAGEのようにより説明的な名前を使用することが推奨されます。次の修正を適用してください：
-PER_PAGE = 50 +ITEMS_PER_PAGE = 50そして、11行目も更新してください：
-@searchables = Kaminari.paginate_array(searchables.uniq).page(params[:page]).per(PER_PAGE) +@searchables = Kaminari.paginate_array(searchables.uniq).page(params[:page]).per(ITEMS_PER_PAGE)app/models/search_result.rb (1)
52-54: N+1クエリの可能性があります。このメソッドは各SearchResultインスタンスの作成時に呼び出されるため、多数の検索結果がある場合にN+1クエリが発生する可能性があります。
コントローラー側で事前にロードしたデータを使用するように変更することを検討してください：
def fetch_login_name(searchable) - User.find_by(id: searchable.try(:user_id))&.login_name + # コントローラーで事前にロードしたユーザーデータを使用 + # この実装は SearchResult の使用方法によって調整が必要 + User.find_by(id: searchable.try(:user_id))&.login_name endまたは、SearchResultの作成時にユーザー情報を渡すようにリファクタリングすることも検討できます。
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/controllers/searchables_controller.rb(1 hunks)app/models/search_result.rb(1 hunks)app/views/searchables/index.html.slim(1 hunks)test/helpers/search_hepler_test.rb(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: hirokiej
PR: fjordllc/bootcamp#8740
File: app/helpers/reports_helper.rb:56-64
Timestamp: 2025-06-29T03:44:15.162Z
Learning: このプロジェクト（fjordllc/bootcamp）では、ja.ymlファイルで一部の単語や文章のみI18n対応されているが、多くのテキストは日本語でハードコーディングされており、完全な国際化対応は行われていない。新しい機能でもI18n対応は不要と判断される。
app/models/search_result.rb (1)
Learnt from: tyrrell-IH
PR: fjordllc/bootcamp#8807
File: app/views/welcome/job_support.html.slim:391-392
Timestamp: 2025-06-21T22:30:20.116Z
Learning: このプロジェクトにはRailsの組み込みmarkdownヘルパが利用できないため、markdownのHTML変換には自前でヘルパを実装する必要がある。
app/views/searchables/index.html.slim (1)
Learnt from: mousu-a
PR: fjordllc/bootcamp#8566
File: app/views/pair_works/_body.html.slim:3-4
Timestamp: 2025-07-01T04:26:24.602Z
Learning: PairWorkモデルにおいて、pair_work.solved?がtrueを返す場合、buddy_idは必ず存在することがビジネスロジックで保証されている。そのため、solved?の条件分岐内でのUser.find(pair_work.buddy_id)は安全に使用できる。
🧬 Code Graph Analysis (3)
test/helpers/search_hepler_test.rb (1)
app/helpers/search_helper.rb (3)
searchable_url(31-41)filtered_message(43-53)created_user(55-61)
app/controllers/searchables_controller.rb (1)
app/models/searcher.rb (1)
search(16-24)
app/models/search_result.rb (3)
app/helpers/search_helper.rb (4)
searchable_url(31-41)searchable_summary(9-19)filtered_message(43-53)created_user(55-61)app/models/concerns/searchable.rb (2)
primary_role(51-56)formatted_updated_at(58-63)app/models/user.rb (1)
avatar_url(706-718)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build_and_test
🔇 Additional comments (8)
app/controllers/searchables_controller.rb (1)
6-16: 実装が適切です。検索処理とN+1クエリの回避のためのデータ事前ロードが適切に実装されています。
current_userコンテキストを使用した検索と、効率的なデータ取得が行われています。app/models/search_result.rb (3)
10-16: 初期化メソッドの改善が適切に行われています。以前のレビューコメントで指摘された複雑な初期化メソッドが、適切に複数のプライベートメソッドに分割されています。可読性と保守性が大幅に向上しています。
66-77: HTML エスケープの実装が適切です。以前のレビューコメントで指摘されたXSS攻撃の脆弱性が修正されています。
ERB::Util.html_escapeを使用してテキストを適切にエスケープしてから、ハイライトを適用しています。
79-86: エラーハンドリングの実装が適切です。以前のレビューコメントで指摘された複雑なI18n呼び出しが、適切なエラーハンドリングを含むプライベートメソッドに分割されています。
app/views/searchables/index.html.slim (4)
19-19: N+1クエリの改善が適切に行われています。以前のレビューコメントで指摘されたN+1クエリの問題が修正されています。コントローラーで事前にロードした
@users_by_idを使用してユーザー情報を取得しています。
46-46: 論理演算子の使用が適切です。
title.presenceを使用してタイトルが存在しない場合にlogin_nameをフォールバックとして使用する実装が適切です。
50-50: セキュリティ対策が適切に実装されています。
sanitizeメソッドを使用してHTMLを適切にサニタイズしています。これにより、XSS攻撃を防ぐことができます。
68-70: safe_joinの使用が適切です。
safe_joinを使用して配列の要素を安全に結合しています。これによりHTML エスケープが適切に処理されます。
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
app/helpers/search_helper.rb (2)
40-50: filtered_message メソッドの実装を確認Product コメントに対するポリシーチェックの実装は適切ですが、エラーハンドリングの追加を検討してください。
def filtered_message(searchable) if searchable.is_a?(Comment) && searchable.commentable_type == 'Product' commentable = searchable.commentable + return '' unless commentable.present? return '該当プラクティスを修了するまで他の人の提出物へのコメントは見れません。' unless policy(commentable).show? || commentable.practice.open_product?
95-99: delete_private_comment! メソッドの命名についてメソッド名に
!が付いているが、実際には破壊的な操作を行っていません。命名の見直しを検討してください。-def delete_private_comment!(searchables) +def filter_private_comments(searchables)app/models/concerns/searchable.rb (1)
83-105: KeywordSearchBuilder の実装についてUser.find_by の呼び出しが検索のたびに発生する可能性があります。パフォーマンスの最適化を検討してください。
def search_user_id_group(username) + return { "#{COLUMN_NAMES_FOR_SEARCH_USER_ID.join('_or_')}_eq" => 0 } if username.blank? + user = User.find_by(login_name: username) { "#{COLUMN_NAMES_FOR_SEARCH_USER_ID.join('_or_')}_eq" => user&.id || 0 } end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/helpers/search_helper.rb(2 hunks)app/models/concerns/searchable.rb(1 hunks)app/models/search_result.rb(1 hunks)app/models/searcher.rb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/models/search_result.rb
- app/models/searcher.rb
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: hirokiej
PR: fjordllc/bootcamp#8740
File: app/helpers/reports_helper.rb:56-64
Timestamp: 2025-06-29T03:44:15.162Z
Learning: このプロジェクト（fjordllc/bootcamp）では、ja.ymlファイルで一部の単語や文章のみI18n対応されているが、多くのテキストは日本語でハードコーディングされており、完全な国際化対応は行われていない。新しい機能でもI18n対応は不要と判断される。
app/helpers/search_helper.rb (1)
Learnt from: tyrrell-IH
PR: fjordllc/bootcamp#8807
File: app/views/welcome/job_support.html.slim:391-392
Timestamp: 2025-06-21T22:30:20.116Z
Learning: このプロジェクトにはRailsの組み込みmarkdownヘルパが利用できないため、markdownのHTML変換には自前でヘルパを実装する必要がある。
🧬 Code Graph Analysis (1)
app/helpers/search_helper.rb (8)
app/models/search_result.rb (1)
include(3-90)app/models/answer.rb (1)
include(3-31)app/models/comment.rb (1)
include(3-69)app/models/practice.rb (2)
include(3-232)body(135-137)app/models/product.rb (1)
include(3-228)app/models/talk.rb (1)
include(3-13)app/models/user.rb (1)
include(3-956)app/helpers/markdown_helper.rb (2)
process_special_case(30-33)markdown_to_plain_text(61-66)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build_and_test
🔇 Additional comments (10)
app/helpers/search_helper.rb (5)
5-5: PolicyHelper の追加が適切ですアクセス制御のための
PolicyHelperの追加により、検索結果のフィルタリング機能が適切に実装されています。
14-14: markdown_to_plain_text の使用が適切です取得した学習内容から、このプロジェクトでは独自のマークダウンヘルパーを使用する必要があることが分かります。
markdown_to_plain_textへの変更は一貫性があり適切です。
29-38: searchable_url メソッドのリファクタリングが改善されていますcase文を使用した実装により、可読性が向上し、Railsのルートヘルパーを活用してURL生成の一貫性が保たれています。
78-93: visible_to_user? メソッドの実装が適切です各モデルタイプに対する適切な可視性チェックが実装されており、管理者権限とユーザー権限の区別が明確になっています。
107-120: filter_by_keywords メソッドの実装が適切です複数キーワードのフィルタリングとuser:プレフィックスの処理が適切に実装されています。ソート処理も含まれており、機能的に完成されています。
app/models/concerns/searchable.rb (5)
9-9: 条件付きスコープ定義が適切です
ActiveRecord::Baseを継承しているクラスのみにスコープを定義することで、適切な防御プログラミングが実現されています。
14-14: KeywordSearchBuilder の使用が適切です検索ロジックの分離により、関心事の分離が適切に実現されています。
18-23: columns_for_keyword_search メソッドのリファクタリングが改善されていますメソッドの実装がより明確になり、kana_name の条件付き追加も適切に処理されています。
40-52: label メソッドの実装が適切です各モデルタイプに対する適切なラベルが定義されており、I18n対応も含まれています。日本語のハードコーディングについては、取得した学習内容から、このプロジェクトでは完全な国際化対応は行われていないことが分かります。
97-100: contains_user_id_column? メソッドの実装が適切ですカラム名の存在チェックが適切に実装されており、効率的な判定が行われています。
nakamu-kazu222
force-pushed
the
feature/change-searchables-not-react_after_cherry-pick
branch
from
25d7fed to
446ca96
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/models/concerns/searchable.rb (1)
33-38: 無限再帰の問題が解決されていません。以前のレビューコメントで指摘された無限再帰の問題が依然として存在します。
SearchResultの場合でもformatted_updated_atメソッドが自分自身を呼び出しています。正しい実装は以下のようになります：
def formatted_updated_at - return @formatted_updated_at if is_a?(SearchResult) + return @formatted_updated_at if is_a?(SearchResult) weekdays = %w[日 月 火 水 木 金 土] updated_at.strftime("%Y年%m月%d日(#{weekdays[updated_at.wday]}) %H:%M") endまたは
SearchResultクラスでこのメソッドをオーバーライドしてください。
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (39)
app/controllers/api/searchables_controller.rb(0 hunks)app/controllers/searchables_controller.rb(1 hunks)app/helpers/markdown_helper.rb(4 hunks)app/helpers/page_tabs/users_helper.rb(1 hunks)app/helpers/search_helper.rb(2 hunks)app/javascript/components/Searchable.jsx(0 hunks)app/javascript/components/Searchables.jsx(0 hunks)app/models/announcement.rb(1 hunks)app/models/answer.rb(1 hunks)app/models/comment.rb(2 hunks)app/models/concerns/commentable.rb(1 hunks)app/models/concerns/searchable.rb(1 hunks)app/models/correct_answer.rb(1 hunks)app/models/event.rb(1 hunks)app/models/page.rb(2 hunks)app/models/practice.rb(1 hunks)app/models/product.rb(1 hunks)app/models/question.rb(1 hunks)app/models/regular_event.rb(1 hunks)app/models/report.rb(1 hunks)app/models/search_result.rb(1 hunks)app/models/search_user.rb(2 hunks)app/models/searcher.rb(1 hunks)app/models/talk.rb(1 hunks)app/models/user.rb(1 hunks)app/views/activity_mailer/assigned_as_checker.html.slim(1 hunks)app/views/api/searchables/_searchable.json.jbuilder(1 hunks)app/views/searchables/index.html.slim(1 hunks)app/views/watches/_watch.html.slim(1 hunks)config/routes/api.rb(0 hunks)db/fixtures/comments.yml(2 hunks)test/fixtures/comments.yml(3 hunks)test/helpers/search_hepler_test.rb(1 hunks)test/integration/api/searchables_test.rb(0 hunks)test/models/comment_test.rb(1 hunks)test/models/searcher_test.rb(1 hunks)test/models/user_test.rb(1 hunks)test/system/current_user/bookmarks_test.rb(1 hunks)test/system/searchables_test.rb(1 hunks)
💤 Files with no reviewable changes (5)
- config/routes/api.rb
- test/integration/api/searchables_test.rb
- app/controllers/api/searchables_controller.rb
- app/javascript/components/Searchables.jsx
- app/javascript/components/Searchable.jsx
✅ Files skipped from review due to trivial changes (2)
- app/models/announcement.rb
- db/fixtures/comments.yml
🚧 Files skipped from review as they are similar to previous changes (28)
- app/models/answer.rb
- app/models/regular_event.rb
- app/models/talk.rb
- app/models/practice.rb
- app/models/report.rb
- app/models/question.rb
- app/models/correct_answer.rb
- app/models/event.rb
- app/models/product.rb
- app/views/watches/_watch.html.slim
- app/models/page.rb
- app/models/comment.rb
- app/views/api/searchables/_searchable.json.jbuilder
- app/views/activity_mailer/assigned_as_checker.html.slim
- test/system/searchables_test.rb
- test/models/user_test.rb
- app/helpers/page_tabs/users_helper.rb
- test/system/current_user/bookmarks_test.rb
- test/models/comment_test.rb
- app/models/concerns/commentable.rb
- test/fixtures/comments.yml
- app/helpers/markdown_helper.rb
- app/models/search_user.rb
- app/controllers/searchables_controller.rb
- test/helpers/search_hepler_test.rb
- app/models/user.rb
- app/views/searchables/index.html.slim
- app/models/search_result.rb
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: hirokiej
PR: fjordllc/bootcamp#8740
File: app/helpers/reports_helper.rb:56-64
Timestamp: 2025-06-29T03:44:15.179Z
Learning: このプロジェクト（fjordllc/bootcamp）では、ja.ymlファイルで一部の単語や文章のみI18n対応されているが、多くのテキストは日本語でハードコーディングされており、完全な国際化対応は行われていない。新しい機能でもI18n対応は不要と判断される。
app/models/concerns/searchable.rb (1)
Learnt from: tyrrell-IH
PR: fjordllc/bootcamp#8807
File: app/views/welcome/job_support.html.slim:391-392
Timestamp: 2025-06-21T22:30:20.116Z
Learning: このプロジェクトにはRailsの組み込みmarkdownヘルパが利用できないため、markdownのHTML変換には自前でヘルパを実装する必要がある。
app/helpers/search_helper.rb (1)
Learnt from: tyrrell-IH
PR: fjordllc/bootcamp#8807
File: app/views/welcome/job_support.html.slim:391-392
Timestamp: 2025-06-21T22:30:20.116Z
Learning: このプロジェクトにはRailsの組み込みmarkdownヘルパが利用できないため、markdownのHTML変換には自前でヘルパを実装する必要がある。
🧬 Code Graph Analysis (1)
app/models/searcher.rb (4)
test/models/searcher_test.rb (1)
current_user(19-21)app/models/comment.rb (1)
include(3-69)app/models/user.rb (1)
include(3-956)app/helpers/search_helper.rb (4)
delete_private_comment!(95-99)filter_by_keywords(107-113)search_model_name(101-105)visible_to_user?(78-93)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build_and_test
🔇 Additional comments (13)
test/models/searcher_test.rb (5)
19-21: テストヘルパーメソッドが適切に実装されています。
current_userヘルパーメソッドにより、テスト全体で一貫したユーザーコンテキストが提供されています。
23-26: HTMLタグ除去ヘルパーが正しく実装されています。検索結果のサマリーからHTMLタグを除去するヘルパーメソッドが適切に実装されており、テストの比較処理で役立ちます。
28-35: 新しいテストケースが検索機能の拡張を適切にカバーしています。ユーザーフィルター無しでの検索結果を検証するテストが適切に実装されています。
37-41: ユーザーフィルタリング機能のテストが正しく実装されています。
user:kimura構文によるユーザーフィルタリングが適切にテストされています。
302-302: フィクスチャのコメント数更新を再確認してください
assert_equal(31, result.size)に合わせて、コメント数が 30 → 31 に更新されているか、フィクスチャデータ側でも反映されているか手動確認をお願いします。先ほど提示した以下のスクリプトでは該当データが出力されなかったため、自動検証が不十分です。
- 対象ファイル（例: test/fixtures/comments.yml）のトップレベルキー数を直接カウント
- または下記スクリプトでレコード数を確認
#!/bin/bash fd 'comments\.(yml|yaml)$' test/fixtures | \ xargs -I {} bash -c "echo {}; grep -E '^[^[:space:]]+: ' {} | wc -l"手動で件数が 31 件になっていることを確認したうえでマージしてください。
app/models/concerns/searchable.rb (2)
9-9: 条件付きスコープ定義が適切に実装されています。ActiveRecord::Baseを継承するクラスでのみスコープが定義されるように適切に制限されています。
80-106: KeywordSearchBuilderモジュールが適切に実装されています。ユーザープレフィックス（
user:）の処理ロジックが明確に分離され、保守性が向上しています。app/models/searcher.rb (3)
16-24: 検索メソッドの拡張が適切に実装されています。
current_userパラメータの必須化、マルチキーワード対応、可視性フィルタリング、SearchResultオブジェクトの返却など、検索機能の大幅な改善が適切に実装されています。
38-47: search_by_user_filter のユーザー未検出時の挙動確認
results_for_allでuser:プレフィックスを指定すると、該当ユーザーが存在しない場合に空配列（[]）が返され、他のタイプ検索が行われず結果がゼロ件になります。意図した仕様かご確認ください。必要に応じて、無効なユーザー名指定時に全タイプ検索へフォールバックするなどの対応をご検討ください。
- ファイル:
app/models/searcher.rb(87–90行目)- 該当箇所:
def search_by_user_filter(username, words) user = User.find_by(login_name: username) or return [] filter_by_keywords(search_by_user_id(user.id), words) end
101-104: SQLインジェクション対策を強化してください。
search_usersメソッドでユーザー入力を直接SQLクエリに使用していますが、より安全な実装を検討してください。以下のようにRansackを使用した実装に変更することを推奨します：
def search_users(words) - User.where(words.map { |_word| 'login_name ILIKE ? OR name ILIKE ? OR description ILIKE ?' } - .join(' AND '), *words.flat_map { |word| ["%#{word}%"] * 3 }) + User.ransack( + groupings: words.map do |word| + { login_name_or_name_or_description_cont: word } + end, + combinator: 'and' + ).result endLikely an incorrect or invalid review comment.
app/helpers/search_helper.rb (3)
5-5: PolicyHelperの追加が適切です。権限チェック機能の追加により、検索結果の可視性制御が適切に実装されています。
29-38: URL生成ロジックが改善されています。case文を使用したURL生成により、コードの可読性と保守性が向上しています。
78-93: 可視性チェック機能が適切に実装されています。ユーザーの権限に基づく検索結果の可視性制御が正しく実装されており、セキュリティが向上しています。
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
app/helpers/search_helper.rb (1)
115-122: result_matches_keyword?メソッドの改善が適切に実装されています。過去のレビューコメントで提案された改善（不要な文字列変換の削減とnilの処理）が適切に実装されています。
🧹 Nitpick comments (2)
app/helpers/search_helper.rb (2)
40-50: filtered_messageメソッドの複雑な条件分岐を簡素化することを検討してください。Productコメントの可視性チェックロジックが複雑になっています。メソッドの責務を分割することで保守性が向上します。
def filtered_message(searchable) - if searchable.is_a?(Comment) && searchable.commentable_type == 'Product' - commentable = searchable.commentable - return '該当プラクティスを修了するまで他の人の提出物へのコメントは見れません。' unless policy(commentable).show? || commentable.practice.open_product? - - return markdown_to_plain_text(searchable.body) - end - - description_or_body = searchable.try(:description) || searchable.try(:body) || '' - markdown_to_plain_text(description_or_body) + return handle_product_comment(searchable) if product_comment?(searchable) + + description_or_body = searchable.try(:description) || searchable.try(:body) || '' + markdown_to_plain_text(description_or_body) end +private + +def product_comment?(searchable) + searchable.is_a?(Comment) && searchable.commentable_type == 'Product' +end + +def handle_product_comment(searchable) + commentable = searchable.commentable + return '該当プラクティスを修了するまで他の人の提出物へのコメントは見れません。' unless policy(commentable).show? || commentable.practice.open_product? + + markdown_to_plain_text(searchable.body) +end
107-113: filter_by_keywordsメソッドのパフォーマンスを最適化することを検討してください。大量の検索結果に対してsort_byとreverseを実行する際のパフォーマンス影響を考慮してください。
def filter_by_keywords(results, words) return results if words.empty? - (results || []).select { |result| words.all? { |word| result_matches_keyword?(result, word) } } - .sort_by(&:updated_at) - .reverse + filtered_results = (results || []).select { |result| words.all? { |word| result_matches_keyword?(result, word) } } + filtered_results.sort_by(&:updated_at).reverse end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/helpers/search_helper.rb(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: hirokiej
PR: fjordllc/bootcamp#8740
File: app/helpers/reports_helper.rb:56-64
Timestamp: 2025-06-29T03:44:15.179Z
Learning: このプロジェクト（fjordllc/bootcamp）では、ja.ymlファイルで一部の単語や文章のみI18n対応されているが、多くのテキストは日本語でハードコーディングされており、完全な国際化対応は行われていない。新しい機能でもI18n対応は不要と判断される。
app/helpers/search_helper.rb (1)
Learnt from: tyrrell-IH
PR: fjordllc/bootcamp#8807
File: app/views/welcome/job_support.html.slim:391-392
Timestamp: 2025-06-21T22:30:20.116Z
Learning: このプロジェクトにはRailsの組み込みmarkdownヘルパが利用できないため、markdownのHTML変換には自前でヘルパを実装する必要がある。
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build_and_test
🔇 Additional comments (5)
app/helpers/search_helper.rb (5)
5-5: PolicyHelperの追加により権限チェック機能が強化されました。検索機能における権限制御の実装が適切に行われています。
14-14: markdown_to_plain_text メソッドの使用が適切です。以前のmarkdown処理方法から統一されたヘルパーメソッドに移行されており、コードの一貫性が向上しています。
29-37: searchable_urlメソッドのリファクタリングが改善されています。if/else文からcase文への変更により、コードの可読性が向上しています。Rails route helpersの使用も適切です。
52-58: created_userメソッドでSearchResultオブジェクトの処理が適切に実装されています。SearchResultオブジェクトと通常のActiveRecordオブジェクトの両方を適切に処理しています。
78-93: visible_to_user? の未処理モデルを確認してください検索機能で
include SearchHelperをしているモデルは以下のとおりです：
Page, User, Talk, Report, RegularEvent, SearchResult, Practice, Question, Answer, Searcher, Event, CorrectAnswer, Announcement, Comment, Productしかし、
visible_to_user?では下記モデルのみを明示的に扱い、SearchResultとCorrectAnswer（さらにサービスクラスのSearcher）はデフォルトの else →falseとなります。
- 明示的にハンドリングされているモデル
Talk, Comment, User, Practice, Page, Event, RegularEvent, Announcement, Report, Product, Question, Answer対応のポイント
- SearchResult・CorrectAnswer（および Searcher）が検索結果として返される場合でも、一律非表示になる意図でよいか
- 必要であればこれらのモデルに対する可視性ロジックを
visible_to_user?に追加する- 今後新しいモデルを
include SearchHelperした際もデフォルトで非表示になることを踏まえ、必要に応じて明示的なケースを追加してください
app/helpers/markdown_helper.rb
Outdated
| text[begin_offset...].strip | ||
| end | ||
|
|
||
| def markdown_to_plain_text(markdown_content) |
There was a problem hiding this comment.
上の方でmarkdownをmd_xxxxと表現していて、toも2と表現しているのでどちらかに合わせた方がいいかもです。
There was a problem hiding this comment.
md_xxxxに揃えるように、md2_plain_textの名前に変更しました
| link: "#{user_micro_reports_path(user, page: user.latest_micro_report_page)}#latest-micro-report", | ||
| count: user.micro_reports.length } | ||
| tabs << { name: '相談部屋', link: talk_path(user.talk) } if current_user.admin? && !user.admin? | ||
| if Rails.env.development? || Rails.env.test? |
There was a problem hiding this comment.
| if Rails.env.development? || Rails.env.test? | |
| if Rails.env.on? %[development test] |
確かこんな感じで書けたと思います。orが入らない分こちらの方がシンプルかもです。
There was a problem hiding this comment.
ご教授いただいたコードにしましたがtest/system/user/micro_reports_test.rb:48でテストエラーになりましたので、if %w[development test].include?(Rails.env)の記法にしました
There was a problem hiding this comment.
There was a problem hiding this comment.
if Rails.env.on? %w[development test]に変更しましたがtest/system/user/micro_reports_test.rb:48でテストエラーになりました
if Rails.env.in? %w[development test]で定義したらエラーがなくなりましたが、こちらでもよろしいでしょうか？
There was a problem hiding this comment.
on?じゃなくてin?の間違えでした。すみません。そちらでOKです。
エラーが出るかどうかではなく、正しいメソッドかどうかで判断するのがいいかもです。
（就職した後で先輩エンジニアに指摘されそうなあるあるな点なので）
There was a problem hiding this comment.
エラーが出るかどうかではなく、正しいメソッドかどうかで判断するのがいいかもです。
（就職した後で先輩エンジニアに指摘されそうなあるあるな点なので）
承知しました
ご教授いただきありがとうございます
|
|
||
| module SearchHelper | ||
| include MarkdownHelper | ||
| include PolicyHelper |
There was a problem hiding this comment.
include PolicyHelperを定義しないとundefined method policy'になってしまいますので使用しております。
include PolicyHelperを定義しないで、policyメソッドを使用する方法を模索した方がよろしいでしょうか？
| end | ||
|
|
||
| def url | ||
| case self |
There was a problem hiding this comment.
Movie, RegularEvent, Talkも必要かなと思います。
There was a problem hiding this comment.
Movie, RegularEvent, Talkを追加しました
nakamu-kazu222
force-pushed
the
feature/change-searchables-not-react_after_cherry-pick
branch
from
720ae9c to
2d88ded
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/system/article/tags_test.rb (1)
51-51: UIの内部実装をテストするよりも、ユーザーが見える部分をテストすることを検討してください。このアサーションの変更により、テストがより内部実装に依存するようになりました。
visible: :allを使用することで、ユーザーには見えない要素をテストしていることになります。system testの本来の目的は、ユーザーの視点から動作を検証することです。タグが正しく追加されたことを確認するために、以下のような代替案を検討してください：
- assert_selector('input[name="article[tag_list]"][value*="注目の記事"]', visible: :all) + # タグがフォーム内で正しく設定されているか確認 + assert_selector('input[name="article[tag_list]"][value*="注目の記事"]', visible: :all) + # または、ユーザーが見える形でタグが表示されているか確認 + assert_text '注目の記事'
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/system/article/tags_test.rb(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`**/*.rb`: # refactoring - まずFat Controllerを避け、次にFat Modelを避ける。 - Serviceクラスの乱用を...
**/*.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の機能があるので使う）
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
test/system/article/tags_test.rb
`test/**/*`: # Test - TestCase名は英語で書く。 - どうしても避けられない時以外にsystem testでsleepは使わない。 # Unit Test model, helper, decorator, view_componentについてはメソッドを追加した場合は必ず対応したUnit TestのTestCaseを1つは書く。
test/**/*: # Test
- TestCase名は英語で書く。
- どうしても避けられない時以外にsystem testでsleepは使わない。
Unit Test
model, helper, decorator, view_componentについてはメソッドを追加した場合は必ず対応したUnit TestのTestCaseを1つは書く。
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
test/system/article/tags_test.rb
🧠 Learnings (1)
📓 Common learnings
Learnt from: hirokiej
PR: fjordllc/bootcamp#8740
File: app/helpers/reports_helper.rb:56-64
Timestamp: 2025-06-29T03:44:15.179Z
Learning: このプロジェクト（fjordllc/bootcamp）では、ja.ymlファイルで一部の単語や文章のみI18n対応されているが、多くのテキストは日本語でハードコーディングされており、完全な国際化対応は行われていない。新しい機能でもI18n対応は不要と判断される。
Learnt from: tyrrell-IH
PR: fjordllc/bootcamp#8807
File: app/views/welcome/job_support.html.slim:391-392
Timestamp: 2025-06-21T22:30:20.116Z
Learning: このプロジェクトにはRailsの組み込みmarkdownヘルパが利用できないため、markdownのHTML変換には自前でヘルパを実装する必要がある。
Learnt from: mousu-a
PR: fjordllc/bootcamp#8566
File: app/views/activity_mailer/matching_pair_work.html.slim:6-9
Timestamp: 2025-06-30T03:27:35.977Z
Learning: このプロジェクトのmd2htmlヘルパーにはサニタイゼーション処理がなく、ユーザー入力をHTMLに変換する際のXSS脆弱性が存在する。この問題は複数のメールViewで共通して発生している。
Learnt from: su-su-su-su
PR: fjordllc/bootcamp#0
File: :0-0
Timestamp: 2025-07-07T05:28:03.676Z
Learning: fjordllc/bootcampプロジェクトでは、fixtureファイル内のERB構文で改行制御文字(-)は使用せず、標準的な<% %>と<%= %>のみを使用する方針が統一されている。<% -%>や<%- %>を使用するとSyntaxErrorが発生する。
⏰ 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
|
お疲れ様です。 また、本PRは #8761 がmainに取り込まれるまで、マージしないようにしていただきたいです |
app/helpers/markdown_helper.rb
Outdated
| end | ||
|
|
||
| def markdown_to_plain_text(markdown_content) | ||
| def md2_plain_text(markdown_content) |
There was a problem hiding this comment.
xxx_to_xxxの略がxxx2xxxなのでmd2plain_textになると思います。
There was a problem hiding this comment.
正確に理解しておりませんでした
修正いたしました
詳細にご教授いただきありがとうございます
|
お疲れ様です。 |
markdownをmd_xxxxと表現していて、toも2と表現しているので揃えた
test/system/user/micro_reports_test.rb:48でテストエラーになったので修正した
nakamu-kazu222
force-pushed
the
feature/change-searchables-not-react_after_cherry-pick
branch
from
73e2be9 to
d698f8f
Compare
komagata
deleted the
feature/change-searchables-not-react_after_cherry-pick
branch
|
ありがとうございます |
2 participants
本PRは、#8801 の実装を移行したものです。
#8801 で多数のコンフリクトが発生したので、本PRおよびブランチを再作成しました。
Issue
概要
検索一覧、検索機能を非React化して、RailsのMVCで代替して同じ機能を実装しました。
変更確認方法
1. 環境構築
feature/change-searchables-not-reactをローカルに取り込むrails db:resetを実行するbin/setupを実行するforeman start -f Procfile.devを実行するmachidaユーザーでログインしている)2. 検索の確認
検索ワードなしでの検索
検索ワードありでの検索
Screenshot
非React化のみなので、画面の変更はなし
Summary by CodeRabbit
新機能
リファクタ
テスト