Conversation
WalkthroughAPI の events エンドポイントと関連 Jbuilder/テスト/ルートを削除し、サーバーサイドのコントローラと Slim ビューでイベント一覧を表示する実装へ移行。EventsController にページネーション定数を追加し、index をクエリ・ページング対応に変更。システムテストを新 UI に合わせて修正。 Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as Browser
participant C as EventsController#index
participant M as Event(Model)
participant V as views/events/index.html.slim
participant P as views/events/_events.html.slim
U->>C: GET /events?page=n
C->>M: with_avatar.includes(:comments, :users)<br/>.order(start_at: :desc).page(page).per(20)
M-->>C: @events (paginated)
C-->>V: render with @events
alt @events.empty?
V-->>U: 空状態を表示
else
V->>P: render 'events/events' with @events
P-->>U: 一覧カード＋ページネーションを描画
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (9)
💤 Files with no reviewable changes (5)
🚧 Files skipped from review as they are similar to previous changes (4)
⏰ 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)
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/events_controller.rbrubocop-minitest extension supports plugin, specify test/system/events_test.rbrubocop-minitest extension supports plugin, specify Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
app/controllers/events_controller.rb (2)
9-11: N+1回避と不要なeager loadの削減（:users → :user へ）一覧ビューでは author の
event.userを参照していますが、includes(:users)は多対多側（参加者）をプリロードしており重い割に未使用です。author を確実にプリロードするため:userを、コメント数のための:commentsは維持で十分です。ついでに同時刻の並び順安定化でid: :descを追加するのがおすすめです。- @events = Event.with_avatar.includes(:comments, :users).order(start_at: :desc).page(params[:page]).per(PAGER_NUMBER) + @events = Event.with_avatar + .includes(:comments, :user) + .order(start_at: :desc, id: :desc) + .page(params[:page]) + .per(PAGER_NUMBER)補足:
- 参加者/補欠者の「件数」だけを表示する用途なら
includesよりも counter_cache（例:events.participants_count,events.waitlist_count）の導入が最も効きます。必要なら別PRで提案します。
4-4: ページサイズ定数の統一を検討他Controller（例: AnnouncementsControllerの25件）と値がばらついています。Kaminariの
default_per_page設定に寄せるか、ApplicationController::PAGER_NUMBER等で共通化すると保守が楽です。app/views/events/index.html.slim (1)
26-34: 空状態分岐はOK。exists? で軽量化も可現状でも問題ありませんが、空の場合のみ軽量化したいなら
@events.exists?を使う手もあります（ただし存在する場合は追加クエリが増えるのでトレードオフ）。app/views/events/_events.html.slim (3)
27-30: time要素のdatetimeはISO8601で
datetimeにはISO8601文字列を渡してください。nil安全も考慮するならセーフナビゲーションを。- time.a-meta(datetime=event.start_at) + time.a-meta datetime=(event.start_at&.iso8601)
11-15: predicateメソッドを利用（wip → wip?）Ruby/Rails慣例として真偽値カラムは
wip?が可読性高いです。- - if event.wip + - if event.wip?
32-40: 件数取得のパフォーマンスについて（任意）
participants.count/waitlist.countはイベントごとにCOUNT(*)が走ります。高トラフィック時は counter_cache の追加（例:participants_count,waitlist_count,comments_count）を検討してください。ビューはそれらを表示するだけにするとI/Oが安定します。必要なら、migration + model修正 + 表示差し替えまでのパッチを用意します。
app/views/shared/_user_icon.html.slim (1)
3-6: localsの存在判定とlazy-loadの付与
defined?(login_name)よりlocal_assigns[:login_name].present?がRails部分テンプレの慣用です。- 画像に
loading="lazy"を付けると一覧の初期ロードが軽くなります。- - if defined?(login_name) && login_name.present? - = image_tag user.avatar_url, alt: user.icon_title, title: user.icon_title, class: img_classes, data: { 'login-name': login_name } + - if local_assigns[:login_name].present? + = image_tag user.avatar_url, alt: user.icon_title, title: user.icon_title, class: img_classes, loading: 'lazy', data: { 'login-name': login_name } - else - = image_tag user.avatar_url, alt: user.icon_title, title: user.icon_title, class: img_classes + = image_tag user.avatar_url, alt: user.icon_title, title: user.icon_title, class: img_classes, loading: 'lazy'
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
app/controllers/api/events_controller.rb(0 hunks)app/controllers/events_controller.rb(1 hunks)app/javascript/components/Events.jsx(0 hunks)app/views/api/events/_event.json.jbuilder(0 hunks)app/views/api/events/index.json.jbuilder(0 hunks)app/views/events/_events.html.slim(1 hunks)app/views/events/index.html.slim(1 hunks)app/views/shared/_user_icon.html.slim(1 hunks)config/routes/api.rb(0 hunks)test/integration/api/events_test.rb(0 hunks)test/system/events_test.rb(4 hunks)
💤 Files with no reviewable changes (6)
- app/javascript/components/Events.jsx
- config/routes/api.rb
- test/integration/api/events_test.rb
- app/controllers/api/events_controller.rb
- app/views/api/events/index.json.jbuilder
- app/views/api/events/_event.json.jbuilder
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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:
test/system/events_test.rbapp/controllers/events_controller.rb
test/**/*
⚙️ CodeRabbit configuration file
test/**/*: # Test
- TestCase名は英語で書く。
- どうしても避けられない時以外にsystem testでsleepは使わない。
Unit Test
model, helper, decorator, view_componentについてはメソッドを追加した場合は必ず対応したUnit TestのTestCaseを1つは書く。
Files:
test/system/events_test.rb
🧠 Learnings (1)
📓 Common learnings
Learnt from: Miya096jp
PR: fjordllc/bootcamp#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.
🧬 Code graph analysis (1)
app/controllers/events_controller.rb (3)
app/controllers/products_controller.rb (2)
before_action(3-156)index(8-13)app/controllers/announcements_controller.rb (2)
before_action(3-119)index(9-15)app/controllers/reports_controller.rb (1)
index(16-20)
🔇 Additional comments (1)
test/system/events_test.rb (1)
251-254: DOMスコープの追加は適切です
within 'ul.card-list'に限定することで誤クリックを防げて安定度が上がっています。Also applies to: 283-286, 315-318, 328-331
sjabcdefin
force-pushed
the
chore/migrate-event-list-to-rails-view
branch
from
b42fedb to
1b490f5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/system/events_test.rb (2)
453-455: タイポによりアサーションが常時パスしてしまう恐れ（正規表現が空）
event[:desription]のキーが誤字です。nilが埋め込まれて//（空の正規表現）となり、意図せず常にマッチします。テストの信頼性に直結するため修正をお願いします。適用例(diff):
- assert has_field? 'announcement[description]', with: /#{event[:desription]}/ + assert has_field? 'announcement[description]', with: /#{event[:description]}/
483-485: 同上：description のキー誤字を修正してくださいこちらも同様の誤字により常時パスのリスクがあります。
適用例(diff):
- assert has_field? 'announcement[description]', with: /#{event[:desription]}/ + assert has_field? 'announcement[description]', with: /#{event[:description]}/
🧹 Nitpick comments (2)
app/views/events/index.html.slim (1)
26-33: nil/空配列両対応の判定・I18n・a11yの微修正提案
@events.empty?はnil時に例外となるため、より堅牢で慣用的なblank?を使うと安心です。あわせて空状態文言のI18n化と、アイコンにaria-hiddenを付与、部分テンプレートへはローカル引数で明示的に渡すと依存関係が明確になります。適用例(diff):
- - if @events.empty? + - if @events.blank? .o-empty-message .o-empty-message__icon - i.fa-regular.fa-sad-tear + i.fa-regular.fa-sad-tear aria-hidden="true" p.o-empty-message__text - | 登録されている特別イベントはありません。 + = t('events.index.empty') # 例: ja.views.events.index.empty - - else - = render 'events/events' + - else + = render 'events/events', events: @events補足: 空状態ブロックは再利用が想定されるなら ViewComponent 化も検討余地ありです。
test/system/events_test.rb (1)
385-393: 空状態表示のシナリオを1本追加すると回帰防止に有効React除去に伴い空一覧のUIが変わったため、「イベントが1件もないときに空状態メッセージが出ること」を検証するE2Eを1本追加すると安心です（既存のページネーション検証と並ぶ網羅性）。
例（概略・実装は運用中のDBリセット戦略に合わせて調整ください）:
test 'show empty state message when no events exist' do visit_with_auth events_path, 'kimura' # 必要に応じてイベントを削除 or テスト用 Tenant/Fixture を用意 assert_text '登録されている特別イベントはありません。' end
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
app/controllers/api/events_controller.rb(0 hunks)app/controllers/events_controller.rb(1 hunks)app/javascript/components/Events.jsx(0 hunks)app/views/api/events/_event.json.jbuilder(0 hunks)app/views/api/events/index.json.jbuilder(0 hunks)app/views/events/_events.html.slim(1 hunks)app/views/events/index.html.slim(1 hunks)config/routes/api.rb(0 hunks)test/integration/api/events_test.rb(0 hunks)test/system/events_test.rb(4 hunks)
💤 Files with no reviewable changes (6)
- test/integration/api/events_test.rb
- app/controllers/api/events_controller.rb
- config/routes/api.rb
- app/views/api/events/_event.json.jbuilder
- app/javascript/components/Events.jsx
- app/views/api/events/index.json.jbuilder
🚧 Files skipped from review as they are similar to previous changes (2)
- app/controllers/events_controller.rb
- app/views/events/_events.html.slim
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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:
test/system/events_test.rb
test/**/*
⚙️ CodeRabbit configuration file
test/**/*: # Test
- TestCase名は英語で書く。
- どうしても避けられない時以外にsystem testでsleepは使わない。
Unit Test
model, helper, decorator, view_componentについてはメソッドを追加した場合は必ず対応したUnit TestのTestCaseを1つは書く。
Files:
test/system/events_test.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)
test/system/events_test.rb (1)
252-254: クリック対象のスコープを狭めたのは良い変更です
within 'ul.card-list'で一覧DOMに限定することで、同名リンクが別領域に存在しても誤クリックを防げます。現状のDOM構造にもフィットしています。Also applies to: 284-286, 316-318, 329-331
|
@matuaya |
|
@sjabcdefin |
matuaya
left a comment
matuaya
left a comment
There was a problem hiding this comment.
遅くなってしまい申し訳ありません🙇♀️
確認いたしました！Approveさせていただきます😊
非React化の対応とても参考になりました😊
|
@matuaya |
sjabcdefin
force-pushed
the
chore/migrate-event-list-to-rails-view
branch
from
344bf42 to
d1aa1f2
Compare
|
@komagata |
3 participants
Issue
概要
変更内容
app/controllers/events_controller.rbでイベント一覧のデータ取得処理をするように修正javascript/components/Events.jsxはapp/views/events/_events.html.slimに移行。app/views/events/index.html.slimで_events.html.slimのrender処理を追加。変更確認方法
chore/migrate-event-list-to-rails-viewをローカルに取り込むforeman start -f Procfile.devでアプリを起動する/events/{id}に遷移すること/users/{id}に遷移すること/users/{id}に遷移することScreenshot
Summary by CodeRabbit
新機能
リファクタ