Conversation
WalkthroughReactベースの通知UIを撤去し、NotificationsControllerのindexでUserNotificationsQueryを用いてサーバー側で通知を取得・ページネーションし、Slimパーシャルでレンダリングするように移行。関連するフロントJSで一括開封後のUI更新を追加。 Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as ブラウザ
participant Controller as NotificationsController
participant Query as UserNotificationsQuery
participant DB as Database
participant View as Slim View / JS
Browser->>Controller: GET /notifications?target=...&status=...&page=...
Controller->>Query: UserNotificationsQuery.call(user:, target:, status:, page:)
Query->>DB: SELECT notifications WHERE user_id=... AND target/ status filters
DB-->>Query: rows
Query-->>Controller: ActiveRecord::Relation / paginated results
Controller->>View: render index with `@notifications`, pagination vars
View-->>Browser: HTML (list partials, filter button, pagination)
Note over Browser,View: （メンターが一括開封した場合）
Browser->>View: click "未読の通知を一括で開く"
View->>Browser: responds (remote action) -> after success, notifications_remove_after_open.js modifies DOM
Browser-->>Browser: removes list, shows empty-state
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related issues
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
smallmonkeykey
force-pushed
the
chore/replace-notifications-to-rails-view
branch
from
5cca5a6 to
5fe1a44
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/javascript/controllers/notifications_controller.js (1)
4-11: 複数のタブを同時に開く際のUX改善を検討してください。多数の通知がある場合、一度に多くのタブを開くとブラウザのポップアップブロッカーが作動する可能性があります。また、ユーザーが意図せず大量のタブを開いてしまうリスクもあります。
以下の改善を検討してください:
- 開くタブの数に上限を設ける（例: 10件まで）
- 開く前に確認ダイアログを表示する
- 開いた件数をユーザーにフィードバックする
改善例:
openAll() { const links = document.querySelectorAll( '.card-list-item .js-unconfirmed-link' ) + if (links.length === 0) { + return + } + if (!confirm(`${links.length}件の通知を開きます。よろしいですか？`)) { + return + } + const maxLinks = 10 + const linksToOpen = Array.from(links).slice(0, maxLinks) - links.forEach((link) => { + linksToOpen.forEach((link) => { window.open(link.href, '_blank', 'noopener') }) + if (links.length > maxLinks) { + alert(`最初の${maxLinks}件を開きました。残り${links.length - maxLinks}件があります。`) + } }app/controllers/notifications_controller.rb (1)
11-20: Query Objectパターンの使用を検討してください。複雑なクエリロジックがコントローラーに記述されています。コーディングガイドラインに従い、rails-patterns gemのQuery機能を使ってQuery Objectパターンを検討することで、コードの再利用性とテスタビリティが向上します。
Based on coding guidelines
例:
# app/queries/filtered_notifications_query.rb class FilteredNotificationsQuery < RailsPatterns::Query queries Notification private def query relation .by_target(options[:target]) .by_read_status(options[:status]) .latest_of_each_link end end # controller latest_notifications = FilteredNotificationsQuery.new( current_user.notifications, target: target, status: status ).to_a @notifications = Notification.with_avatar .from(latest_notifications, :notifications) .order(created_at: :desc) .page(params[:page]) .per(20)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/controllers/notifications_controller.rb(1 hunks)app/javascript/components/Notification.jsx(0 hunks)app/javascript/components/Notifications.jsx(0 hunks)app/javascript/controllers/notifications_controller.js(1 hunks)app/views/notifications/_filter_button.html.slim(1 hunks)app/views/notifications/_notification.html.slim(1 hunks)app/views/notifications/index.html.slim(1 hunks)
💤 Files with no reviewable changes (2)
- app/javascript/components/Notifications.jsx
- app/javascript/components/Notification.jsx
🧰 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:
app/controllers/notifications_controller.rb
**/*.js
⚙️ CodeRabbit configuration file
**/*.js: - どうしても避けられない時以外に新規にVue.js, Reactのコードを追加しない。
- fetchメソッドの代わりにrequest.jsを使う。
Files:
app/javascript/controllers/notifications_controller.js
⏰ 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 (3)
app/views/notifications/_filter_button.html.slim (1)
1-12: フィルターナビゲーションの実装が適切です。アクティブ状態の判定ロジックが正しく、
targetパラメータも適切に保持されています。Rails viewへの移行として良い実装です。app/views/notifications/_notification.html.slim (1)
1-30: Reactからの移行が適切に実装されています。セマンティックHTMLを使用し、アクセシビリティにも配慮された実装です。Stimulusコントローラーとの連携用クラス（
.js-unconfirmed-link）も適切に配置されています。app/views/notifications/index.html.slim (1)
47-76: ReactからRails viewへの移行が成功しています。以下の点が適切に実装されています:
- フィルターボタンとの統合
- 空状態の適切な処理
- ページネーションの配置
- Stimulusコントローラーとの連携
- メンター専用機能の権限チェック
各パーシャルとコントローラーの統合が正しく、一貫性のある実装になっています。
smallmonkeykey
requested review from
komagata and
okuramasafumi
and removed request for
komagata and
okuramasafumi
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/controllers/notifications_controller.rb (1)
15-24: QueryObject パターンの適用を検討してください。通知一覧の取得ロジックが複雑になっています。サブクエリを使ったクエリ構築は将来的な保守性や再利用性の観点から、QueryObject パターンへの切り出しを検討することをお勧めします。
コーディングガイドラインに従い、
rails-patternsgem の Query 機能を活用できます。参考実装例（
app/queries/notifications/latest_by_user_query.rb）:module Notifications class LatestByUserQuery < Patterns::Query queries Notification def initialize(user:, target: nil, status: nil, page: 1, per_page: NotificationsController::PER_PAGE) @user = user @target = target @status = status @page = page @per_page = per_page end def query latest_notifications = @user.notifications .by_target(@target) .by_read_status(@status) .latest_of_each_link Notification.with_avatar .from(latest_notifications, :notifications) .order(created_at: :desc) .page(@page) .per(@per_page) end end endコントローラ側の使用例:
def index @target = params[:target] target = params[:target].presence&.to_sym target = nil unless ALLOWED_TARGETS.include?(target) status = params[:status] @notifications = Notifications::LatestByUserQuery.new( user: current_user, target: target, status: status, page: params[:page] ).run endAs per coding guidelines.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/controllers/notifications_controller.rb(1 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/notifications_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 (2)
app/controllers/notifications_controller.rb (2)
5-5: PER_PAGE の値が小さすぎる可能性があります。
PER_PAGE = 3は本番環境の通知一覧としては極端に小さい値です。ユーザーは多数のページをクリックする必要があり、UXが低下します。通常は 10〜50 件程度が適切です。テスト目的であれば問題ありませんが、本番環境向けであれば値の見直しをお勧めします。
⛔ Skipped due to learnings
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`が通知一覧のフィルタ対象ではないためで、意図的な設計判断である。
13-13: status パラメータにもバリデーションを追加してください。
targetパラメータにはホワイトリストによる検証が追加されていますが、statusパラメータは未検証のままby_read_statusスコープに渡されています。スコープの実装から、status == 'unread'以外の値はすべて同じ動作（読み取り済みの通知を返す）として処理されることが確認されました。予期しない値が渡された場合の動作を明確にするため、許可された値のみを受け付けるバリデーションを追加することを推奨します。改善例:
ALLOWED_TARGETS = %i[mention comment following_report check product watch].freeze + ALLOWED_STATUSES = %w[read unread].freeze PER_PAGE = 3 def index @target = params[:target] target = params[:target].presence&.to_sym target = nil unless ALLOWED_TARGETS.include?(target) - status = params[:status] + status = params[:status].presence + status = nil unless ALLOWED_STATUSES.include?(status)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/views/notifications/index.html.slim (1)
47-55: フィルターボタンの重複レンダリングを改善できます。
filter_buttonパーシャルが条件分岐の両方のブランチでレンダリングされています。条件文の前に移動することでコードの重複を削減できます。次のようにリファクタリングを検討してください：
+ = render 'filter_button' - if @notifications.empty? - = render 'filter_button' .o-empty-message .o-empty-message__icon .i.fa-regular.fa-smile p.o-empty-message__text = params[:status] == 'unread' ? '未読の通知はありません' : '通知はありません' - else - = render 'filter_button' .page-content.loaded[data-controller="notifications"]
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/views/notifications/index.html.slim(1 hunks)
⏰ 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/views/notifications/index.html.slim (1)
56-56: すべての依存ファイルとアクションの存在を確認しました。検証結果：
- ✓ Stimulusコントローラ
app/javascript/controllers/notifications_controller.js- 存在確認- ✓
openAllアクション - 存在確認（4行目）- ✓ パーシャル
app/views/notifications/_notification.html.slim- 存在確認すべての依存関係が正しく実装されており、参照に問題はありません。
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/views/notifications/index.html.slim (1)
23-23:@targetを ALLOWED_TARGETS に対して検証してください。コントローラーの line 10 で
@target = params[:target]と無検証で生の値を保存しており、ビューの line 23 でt("notification.#{@target}")に補間されています。同じパラメータに対して line 11-12 で別のtarget変数が ALLOWED_TARGETS に対して検証されているため、@targetも同様に検証する必要があります。無効な値が渡されると、翻訳キー "notification.invalid_value" など存在しないキーが参照され、翻訳エラーが発生します。修正：
app/controllers/notifications_controller.rbの line 10 を、検証ロジックを含む以下のように変更してください。target = params[:target].presence&.to_sym target = nil unless ALLOWED_TARGETS.include?(target) @target = target
🧹 Nitpick comments (1)
app/views/notifications/index.html.slim (1)
56-58: ページネーションブロックの重複（オプショナル）。ページネーションが上下に表示される実装は良いUXですが、同じコードブロックが2回記述されています。必要に応じて部分テンプレートに抽出することで、将来の変更時の保守性が向上します。
部分テンプレートへの抽出例：
/ app/views/notifications/_pagination.html.slim を作成 - if notifications.total_pages > 1 nav.pagination = paginate notificationsそしてビューで：
- - if @notifications.total_pages > 1 - nav.pagination - = paginate @notifications + = render 'pagination', notifications: @notificationsAlso applies to: 73-75
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/views/notifications/index.html.slim(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
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`が通知一覧のフィルタ対象ではないためで、意図的な設計判断である。
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build_and_test
🔇 Additional comments (5)
app/views/notifications/index.html.slim (5)
64-65: 過去のインデント問題が修正されています。前回のレビューで指摘された
hr.a-border-tintの下に.card-footerがネストされていた問題は修正されています。現在は両要素が同じインデントレベルで兄弟要素として正しく配置されています。
47-47: フィルターボタンの部分テンプレート描画。
filter_button部分テンプレートの描画は適切です。
63-71: メンター専用の一括開封ボタンが適切に実装されています。権限チェック（
current_user.mentor?）とStimulusコントローラーへのアクション接続が正しく実装されています。PRの目的通り、メンターのみが未読通知を一括で開くことができます。
54-75: React から Rails + Stimulus への移行が適切に実装されています。全体的な構造は明確で、Rails の規約に従っています。Stimulus コントローラーの統合も適切で、React コンポーネントからの移行として優れた実装です。
55-62: コードの N+1 対策は既に実装されています。検証の結果、
_notification部分テンプレートでnotification.senderにアクセスする際の N+1 クエリは既に防止されています。コントローラーで使用されているwith_avatarスコープ（Notification モデル 50 行目）がpreload(sender: { avatar_attachment: :blob })で適切に関連を事前読み込みしているため、追加のクエリは発生しません。notification.path、notification.message、その他のアクセスもすべてデータベース列またはルーティングヘルパーのみで、追加のクエリを引き起こしません。
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/controllers/notifications_controller.rb (1)
9-25: クエリロジックは正しく実装されていますが、将来的にはQuery Objectパターンを検討してください。サブクエリを使用した実装は適切ですが、クエリロジックが複雑になっています。現時点では許容範囲内ですが、今後さらに複雑になる場合は、コーディングガイドラインに従ってQuery Objectパターン（rails-patterns gem）への移行を検討することをお勧めします。
Based on coding guidelines
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/controllers/notifications_controller.rb(1 hunks)app/views/notifications/index.html.slim(1 hunks)test/system/notifications_test.rb(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/views/notifications/index.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/notifications_test.rbapp/controllers/notifications_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/notifications_test.rb
🧠 Learnings (2)
📓 Common learnings
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`が通知一覧のフィルタ対象ではないためで、意図的な設計判断である。
📚 Learning: 2025-07-15T12:58:02.735Z
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`が通知一覧のフィルタ対象ではないためで、意図的な設計判断である。
Applied to files:
app/controllers/notifications_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 (3)
test/system/notifications_test.rb (2)
67-101: ページネーション操作の変更が適切です。
click_link_or_buttonを使用することで、Rails viewでページネーションがリンクとボタンのどちらでレンダリングされても対応できます。React版からの移行に適した変更です。
127-158: クエリ文字列を含むページネーション操作も適切です。複数のクエリパラメータを含むページネーション操作でも
click_link_or_buttonを使用することで、Rails viewへの移行後も正しく動作します。Also applies to: 160-191, 222-254
app/controllers/notifications_controller.rb (1)
4-6: 定数定義が適切です。
ALLOWED_TARGETSとPER_PAGEの定数化により、コードの保守性が向上しています。PER_PAGE = 20はテストの期待値とも一致しています。
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/controllers/notifications_controller.rb (1)
15-24: Query Objectパターンの検討を推奨します。通知一覧のクエリロジックが複雑化しており、中間変数
latest_notificationsを経由して.from()でサブクエリを構成しています。コーディングガイドラインに従い、この複雑なActiveRecordクエリはQuery Objectパターンへの抽出を検討することを推奨します。利点：
- コントローラがスリムになり、Fat Controllerを回避できます
- クエリロジックの再利用性が向上します（API、バックグラウンドジョブなど）
- テスタビリティが向上します
- 将来の変更時の影響範囲が限定されます
以下のようにQuery Objectとして抽出できます：
# app/queries/notifications/latest_by_user_query.rb module Notifications class LatestByUserQuery < ApplicationQuery queries Notification def initialize(user, target: nil, status: nil) @user = user @target = target @status = status end def call latest_notifications = @user.notifications .by_target(@target) .by_read_status(@status) .latest_of_each_link Notification.with_avatar .from(latest_notifications, :notifications) .order(created_at: :desc) end end endコントローラは以下のようにシンプルになります：
def index @target = params[:target] target = params[:target].presence&.to_sym target = nil unless ALLOWED_TARGETS.include?(target) status = params[:status] @notifications = Notifications::LatestByUserQuery .call(current_user, target: target, status: status) .page(params[:page]) .per(PER_PAGE) endBased on coding guidelines
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/controllers/notifications_controller.rb(1 hunks)test/system/notifications_test.rb(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/system/notifications_test.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/notifications_controller.rb
🧠 Learnings (1)
📚 Learning: 2025-07-15T12:58:02.735Z
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`が通知一覧のフィルタ対象ではないためで、意図的な設計判断である。
Applied to files:
app/controllers/notifications_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 (2)
app/controllers/notifications_controller.rb (2)
11-13: パラメータバリデーションが適切に実装されています。過去のレビューで指摘された
to_symの安全性の問題が、ALLOWED_TARGETSによる検証で適切に解決されています。予期しない値が渡された場合はnilにフォールバックする実装で、セキュリティ上の懸念は解消されています。
4-4: 検証完了：ターゲット名は正確で一貫しています。Notificationモデルの
TARGETS_TO_KINDSと比較したところ、ALLOWED_TARGETSの定義は完全に一致しており、問題はありません：
watchingは正しい名称（watchではない）announcementは有効なターゲット種別- すべてのターゲットがモデル定義と一致
モデルの
TARGETS_TO_KINDSにはannouncement、mention、comment、check、watching、following_reportが定義されており、これらはコントローラのALLOWED_TARGETSと完全に対応しています。また、以前の学習情報から、
training_completedがこのハッシュに含まれていないのは、通知一覧のフィルタ対象ではないための意図的な設計判断であることが確認できます。
|
@jun-kondo |
|
@smallmonkeykey |
|
@jun-kondo 以下質問に答えました。
はい、合っています！
はい、今回はベルの部分は無関係です！ |
|
@smallmonkeykey また、ご回答ありがとうございます。 |
#9184 |
There was a problem hiding this comment.
確認しました🙋♂️
Queryオブジェクトパターンを使ったのですね!!よいと思いますー
いくつかコメント致しましたのでご確認お願い致します🙇♂️
Stimulusではなくなったので、プルリクエストのタイトルも変更したほうが良さそうです。
もう一点ですが、レビュー依頼の段階でお願いすべきだったのですが、コミットの整理の検討をお願いしたいです。
「ある機能を実装するコミットA」と「コミットAの内容を修正するコミットB」があればAとBは合わせてしまったほうがよいと思います。でないと同じ箇所がコミット毎に何度も変わってしまうのでコミット毎のレビューが大変ですし、マージ後のコミット履歴もわかりづらくなってしまいます。
参考までにこちらの記事を コミットは作業ログではない！ #Git - Qiita
このPRで実現したいことは通知機能一覧ページの非React化であり、ざっくり分けると
- 通知一覧画面の実装を react から rails view に変更
- 通知の絞り込み用にQueryオブジェクト追加
- 未読通知を一括で開くボタンをreactからVanillaJSに変更
だと思います。最終的にstimulusを使わなかったので最初から使わなかった体でコミットを整理してみては如何でしょうか?
そうすれば前半の変更履歴がコミット単位でレビューしやすくなると思います。
提案としては以下になります。
- stimulusで実装していたときの一連のコミットを整理する
- タイポやLintチェック漏れや変更漏れの修正コミットはそれぞれ大本の変更コミットに含める
- その後レビューによる修正コミットがあればそれを積んでいく
やり方についてはFBCのhirokiejさんのブログが参考になりましたので貼っておきます
以上です。宜しくお願い致します🙇♂️
追記) 自分のrebase案を書いてみました。これでコミット数が15から8~9個ぐらいに圧縮出来ると思います。
- 38a0ff4 からstimulusの変更箇所を削除
- 05e39e7 のコミット自体が不要になるのでDropできる
- 9878e69 は 38a0ff4 の編集漏れなので 38a0ff4 に統合
- 7571996 と 3cd0bee は両方テストに関する変更でコミットメッセージもほぼ同じなので統合する。このうち定数に関する修正は 58c895d に含める
- 58c895d で定数を追加したが、 60592da で結局削除するのでDropしても良いかもしれない
- 013f437 と 2bb5e7a は abcbe51 の修正漏れなので abcbe51 統合する
- 8d69235 は 0d8edaa の修正漏れなので 0d8edaa 統合する
| p.o-empty-message__text | ||
| = params[:status] == 'unread' ? '未読の通知はありません' : '通知はありません' | ||
| - else | ||
| .page-content.loaded#notifications |
There was a problem hiding this comment.
JavaScriptファイルやテストで使われていなさそうなので、疑問だったのですがloadedを追加した理由を教えて頂けますか?🙋♂️
There was a problem hiding this comment.
Reactの中で<div id="notifications" className="page-content loaded">という箇所があったので、そのまま残してつけておりました。
また、こちらをつけないとページネーションのテストで不安定になっていたために付けました。
現在はこちらのテストがmainから消されており、不要なため削除します。ありがとうございます！
| li.pill-nav__item | ||
| = link_to '全て', | ||
| notifications_path(target: params[:target]), | ||
| class: "pill-nav__item-link #{params[:status] == 'unread' ? '' : 'is-active'}" |
There was a problem hiding this comment.
もともとNotifications.jsxで一緒になっていた部分ですが分割されたのですね。わかりやすくなってとても良いと思いました🙋♂️
| ) | ||
| if (!allOpenButton) return | ||
|
|
||
| allOpenButton.addEventListener('click', () => { |
There was a problem hiding this comment.
#9184 (review)
を読んでいて思ったのですが、通知に関してはリロードすれば未読通知や「一括で開く」ボタンは消えるのではないかなと思いました(違っていたらすみません)
現状だと、app/javascript/unconfirmed-links-open.jsとapp/javascript/notifications_remove_after_open.jsで処理が別れていますが、上記のプルリクエストのようにapp/javascript/unconfirmed-links-open.js内でlocation.reload()を行えばDOMの削除や置き換えが不要になるのではないかなと思いましたがいかがでしょうか?
There was a problem hiding this comment.
コメントありがとうございます！
ページリロードについては検討したのですが、ページをリロードするとすべてを読み込むため、一部差し換えのほうが処理負担がかからないのではないかと思いこの実装にしてみました。
Junさんはこの点についてどう思われますか？
リロードのほうがすっきりするとは思いますので、特に問題なければリロードに差し替えます！
There was a problem hiding this comment.
ページリロードについては検討したのですが、ページをリロードするとすべてを読み込むため、一部差し換えのほうが処理負担がかからないのではないかと思いこの実装にしてみました。
確かにその通りだと思います🙋♂️
一方で、一括で開くボタンを押した時点で複数タブでページを読み込み始めているので、もう一枚リロードによる読み込みが増えても変わらないのでは？とも思いました。
もう一点理由というか気になった点があり、#9184 で未確認のリンクを一括で開くボタン(UnconfirmedLink.jsx)をバニラJSで実装しており、通知一覧ページのボタンもそれらの修正に含まれます。
本来なら一括で開くボタンの修正は#9184 で行うべきで、このPRで修正しなくて良い箇所だと思いました。安定を取るなら#9184 マージされるのを待っても良い気がします。そのまま進めるなら未読通知を一括で開くボタンを押したときの処理はなるべく#9184 の実装に合わせた方が後々コンフリクトが起きた際も修正しやすいのではと思いました。
ちょっとこのあたり認識が間違っていたら申し訳ありませんが気になったのでコメント致しました🙇♂️
There was a problem hiding this comment.
確かにその通りだと思います🙋♂️
一方で、一括で開くボタンを押した時点で複数タブでページを読み込み始めているので、もう一枚リロードによる読み込みが増えても変わらないのでは？とも思いました。
この点について改めて考えたのですが、やはりリロードではなく部分更新の方が良いのではと思いました。
例えば、Reactなどのクライアント側で状態を管理する実装を想定した場合、ページ遷移を伴わずに要素を操作するケースでは、リロードではなく、状態更新によって表示を切り替えるのが一般的だと思います。
リロードすれば結果的に画面は更新されますが、それを前提にしてしまうと、クライアント側で状態や表示を制御している意味が薄れてしまうのではと思いました。
本来なら一括で開くボタンの修正は#9184 で行うべきで、このPRで修正しなくて良い箇所だと思いました。
こちらに関してなのですが現在のmainの一括で開くJSの処理app/javascript/unconfirmed-links-open.jsは以下のようになっています。
document.addEventListener('DOMContentLoaded', () => {
const allOpenButton = document.querySelector(
'#js-shortcut-unconfirmed-links-open'
)
if (allOpenButton) {
allOpenButton.addEventListener('click', () => {
const links = document.querySelectorAll(
'.card-list-item .js-unconfirmed-link'
)
links.forEach((link) => {
window.open(link.href, '_target', 'noopener')
})
})
}
})
#9184 のapp/javascript/unconfirmed-links-open.jsを確認したところ、
setTimeout(() => {
location.reload()
}, 100)
が追加されおりjunさんのおっしゃる通り、このファイルの変更の箇所はいらなくなると思います。
しかし現在のmainでは一括処理のみのJSしかない形なので、画面を変更する箇所は必要だと思いました。
またどちらが先にマージされるかはわからないですが、仮にどちらがされてもこのファイルを削除するだけで対応できると思うので、現時点では大きなバッティングや解消が難しいコンフリクトにはならないと考えています！
もし認識がずれていればご指摘いただけると助かります。
smallmonkeykey
force-pushed
the
chore/replace-notifications-to-rails-view
branch
2 times, most recently
from
33f44df to
ab8746d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/queries/user_notifications_query.rb (1)
32-43: 3つ目のテストでreadのデフォルト値に依存している点を明示すると、将来の変更に強くなりますこのテストでは
status: 'unread'を指定していますが、comment_notification/check_notificationどちらもreadを明示しておらず、Notification モデルのデフォルト値に挙動が依存しています。
将来的に migration でreadのデフォルトが変更された場合にテストの意味が揺らぐ可能性があるので、必要であれば両方にread: falseを付けておくとテストの意図がより伝わりやすくなると思います。
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
app/controllers/notifications_controller.rb(1 hunks)app/javascript/components/Notification.jsx(0 hunks)app/javascript/components/Notifications.jsx(0 hunks)app/javascript/notifications_remove_after_open.js(1 hunks)app/javascript/packs/application.js(1 hunks)app/queries/user_notifications_query.rb(2 hunks)app/views/notifications/_filter_button.html.slim(1 hunks)app/views/notifications/_notification.html.slim(1 hunks)app/views/notifications/index.html.slim(1 hunks)test/queries/user_notifications_query.rb(1 hunks)test/system/notifications_test.rb(1 hunks)
💤 Files with no reviewable changes (2)
- app/javascript/components/Notifications.jsx
- app/javascript/components/Notification.jsx
🚧 Files skipped from review as they are similar to previous changes (7)
- app/queries/user_notifications_query.rb
- app/javascript/packs/application.js
- app/views/notifications/index.html.slim
- app/controllers/notifications_controller.rb
- app/views/notifications/_notification.html.slim
- test/system/notifications_test.rb
- app/views/notifications/_filter_button.html.slim
🧰 Additional context used
📓 Path-based instructions (6)
**/*.rb
📄 CodeRabbit inference engine (AGENTS.md)
Ruby code should use 2-space indentation, snake_case for method names, and CamelCase for class names, enforced by RuboCop (
.rubocop.yml)
Files:
test/queries/user_notifications_query.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/queries/user_notifications_query.rb
test/**
📄 CodeRabbit inference engine (AGENTS.md)
Keep tests deterministic and use fixtures stored in
test/fixtures/for test data
Files:
test/queries/user_notifications_query.rb
test/**/*
⚙️ CodeRabbit configuration file
test/**/*: # Test
- TestCase名は英語で書く。
- どうしても避けられない時以外にsystem testでsleepは使わない。
Unit Test
model, helper, decorator, view_componentについてはメソッドを追加した場合は必ず対応したUnit TestのTestCaseを1つは書く。
Files:
test/queries/user_notifications_query.rb
app/**/*.{rb,js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Rails app code should be organized in
app/directory with subdirectories:models/,controllers/,views/,jobs/,helpers/, and frontend code underjavascript/(Shakapacker)
Files:
app/javascript/notifications_remove_after_open.js
app/javascript/**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
JavaScript/TypeScript code in
app/javascript/should be linted with ESLint and formatted with Prettier viayarn lintscripts; use React 17 and Shakapacker/Webpack 5
Files:
app/javascript/notifications_remove_after_open.js
**/*.js
⚙️ CodeRabbit configuration file
**/*.js: - どうしても避けられない時以外に新規にVue.js, Reactのコードを追加しない。
- fetchメソッドの代わりにrequest.jsを使う。
Files:
app/javascript/notifications_remove_after_open.js
🧠 Learnings (5)
📓 Common learnings
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/models/graduation_notifier.rb:23-28
Timestamp: 2025-09-12T21:16:47.639Z
Learning: Rails upgrade PRにおいて、configuration systemの変更やwebhook設定の改善提案も、アップグレードに直接関連しない場合は別PRで対応すべきである。PRのスコープ維持が重要。
Learnt from: tyrrell-IH
Repo: fjordllc/bootcamp PR: 9306
File: app/javascript/components/Bookmarks.jsx:248-265
Timestamp: 2025-11-17T00:46:30.794Z
Learning: fjordllc/bootcamp プロジェクトでは、Reactからバニラ JavaScript への移行作業が進行中である。そのため、新しいReactファイルの作成は避け、既存のReactコンポーネント内で完結する実装が推奨される。BookmarksInDashboard.jsx は issue #9045 で削除予定。
Learnt from: Miya096jp
Repo: fjordllc/bootcamp PR: 9102
File: app/controllers/mentor/buzzes_controller.rb:0-0
Timestamp: 2025-09-04T01:39:22.261Z
Learning: fjordllc/bootcamp プロジェクトでは、更新処理の成功時の通知メッセージについて、I18n を使用せずに日本語文字列を直接記述する方針で一貫性を保っている。
Learnt from: reckyy
Repo: fjordllc/bootcamp PR: 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`が通知一覧のフィルタ対象ではないためで、意図的な設計判断である。
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:19-23
Timestamp: 2025-09-12T21:17:26.029Z
Learning: Rails アップグレードPRにおいて、config_for(:secrets) を使った 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のスコープ維持が重要。
Learnt from: sjabcdefin
Repo: fjordllc/bootcamp PR: 9090
File: app/javascript/user-sns.js:2-25
Timestamp: 2025-08-25T08:00:11.369Z
Learning: app/javascript/user-sns.js において、アクセシビリティ改善（aria-label属性の追加など）は他の箇所との整合性を保つため、別途統一して対応する方針である
📚 Learning: 2025-07-15T12:58:02.735Z
Learnt from: reckyy
Repo: fjordllc/bootcamp PR: 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`が通知一覧のフィルタ対象ではないためで、意図的な設計判断である。
Applied to files:
test/queries/user_notifications_query.rb
📚 Learning: 2025-11-17T00:46:30.794Z
Learnt from: tyrrell-IH
Repo: fjordllc/bootcamp PR: 9306
File: app/javascript/components/Bookmarks.jsx:248-265
Timestamp: 2025-11-17T00:46:30.794Z
Learning: fjordllc/bootcamp プロジェクトでは、Reactからバニラ JavaScript への移行作業が進行中である。そのため、新しいReactファイルの作成は避け、既存のReactコンポーネント内で完結する実装が推奨される。BookmarksInDashboard.jsx は issue #9045 で削除予定。
Applied to files:
app/javascript/notifications_remove_after_open.js
📚 Learning: 2025-09-12T21:18:00.834Z
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で対応すべきである。
Applied to files:
app/javascript/notifications_remove_after_open.js
📚 Learning: 2025-09-28T00:36:43.971Z
Learnt from: jun-kondo
Repo: fjordllc/bootcamp PR: 9104
File: app/javascript/bookmark-button.js:5-11
Timestamp: 2025-09-28T00:36:43.971Z
Learning: fjordllc/bootcampアプリケーションでは現在、各ページにブックマークボタンは1つのみ存在し、Turbo Driveは未使用のため、querySelector での単一要素初期化とDOMContentLoadedのみのイベントリスナーで十分である。
Applied to files:
app/javascript/notifications_remove_after_open.js
⏰ 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 (3)
app/javascript/notifications_remove_after_open.js (2)
1-8: 初期化処理とガードは適切ですDOMContentLoadedでの初期化、要素の存在チェック、早期リターンによるガード処理は、プロジェクトの他のファイル（bookmark-button.js等）と一貫性があり、適切に実装されています。
10-26: DOM操作アプローチについて継続的な議論を確認してください以前のコメントで、
location.reload()を使用する代替アプローチが提案されている場合は、DOM要素の削除と置き換えによる現在の実装とページリロードによる実装のトレードオフについて、チームで最終的な方針を確認することをお勧めします。オプション：DOM API使用の検討
innerHTMLでの一括置換は機能的には問題ありませんが、DOM APIを使用した要素作成の方が、将来的な保守性やテストのしやすさが向上する可能性があります。Likely an incorrect or invalid review comment.
test/queries/user_notifications_query.rb (1)
6-43: UserNotificationsQuery のテスト設計が責務に沿っていて分かりやすいですユーザー単位の絞り込み・未読ステータス・target（'check'）ごとのフィルタをそれぞれ別テストで検証していて、クエリオブジェクトの期待挙動が読み取りやすい構成になっていると思います。
smallmonkeykey
force-pushed
the
chore/replace-notifications-to-rails-view
branch
from
ab8746d to
209d5ac
Compare
未読通知ボタンの動作をStimulusからJS実装にした stimulusの実装の部分は不要になったので削除 一括で開く時は、元のタブの通知を削除 インデントの修正とフィルターボタンの重複レンダリングを改善 フィルターボタンが消えていたので反映されるようにした コードスタイルを修正
smallmonkeykey
force-pushed
the
chore/replace-notifications-to-rails-view
branch
from
209d5ac to
93e59c0
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/queries/user_notifications_query.rb (1)
8-28:validated_targetのキー型依存を確認しておいてください
validated_targetでNotification::TARGETS_TO_KINDS.key?(@target)を使うのは安全側に寄せられていて良いのですが、ここは「ハッシュのキーの型」に依存する点だけ少し気になりました。
- コントローラ側から渡している
targetはparams[:target]由来で String の可能性が高いです。- 一方で
Notification::TARGETS_TO_KINDSのキーが Symbol の場合、key?('report')は常にfalseになり、結果的に常にnil（= 絞り込みなし）になる恐れがあります。もし
TARGETS_TO_KINDSのキーが Symbol であれば、どこか一箇所（コントローラ or ここ）でto_symなどにより型を揃えておくと挙動が明確になると思います。逆にキーが String で統一されているのであれば現在の実装で問題ないので、その前提を一度確認しておいてください。
app/views/notifications/_notification.html.slim (1)
1-30: 既存挙動をよく踏襲できている通知行のテンプレートです未読・既読のクラス切り替えや「未読」バッジ、
js-unconfirmed-linkクラスでの JS フックなど、React 時代の挙動をそのままサーバーレンダリングに移せていて違和感のない構造だと思います。細かい話ですが、もし他の画面で
timeタグのdateTimeを ISO8601 文字列（created_at.iso8601など）に揃えているようなら、ここも合わせておくと HTML 的にはより厳密になります（現状でも実害はほぼないと思います）。
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
app/controllers/notifications_controller.rb(1 hunks)app/javascript/components/Notification.jsx(0 hunks)app/javascript/components/Notifications.jsx(0 hunks)app/javascript/notifications_remove_after_open.js(1 hunks)app/javascript/packs/application.js(1 hunks)app/queries/user_notifications_query.rb(2 hunks)app/views/notifications/_filter_button.html.slim(1 hunks)app/views/notifications/_notification.html.slim(1 hunks)app/views/notifications/index.html.slim(1 hunks)test/queries/user_notifications_query.rb(1 hunks)
💤 Files with no reviewable changes (2)
- app/javascript/components/Notification.jsx
- app/javascript/components/Notifications.jsx
🚧 Files skipped from review as they are similar to previous changes (4)
- app/controllers/notifications_controller.rb
- app/javascript/notifications_remove_after_open.js
- app/javascript/packs/application.js
- test/queries/user_notifications_query.rb
🧰 Additional context used
📓 Path-based instructions (3)
**/*.slim
📄 CodeRabbit inference engine (AGENTS.md)
Slim templates should be linted according to
config/slim_lint.yml
Files:
app/views/notifications/_notification.html.slimapp/views/notifications/index.html.slimapp/views/notifications/_filter_button.html.slim
app/**/*.{rb,js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Rails app code should be organized in
app/directory with subdirectories:models/,controllers/,views/,jobs/,helpers/, and frontend code underjavascript/(Shakapacker)
Files:
app/queries/user_notifications_query.rb
**/*.rb
📄 CodeRabbit inference engine (AGENTS.md)
Ruby code should use 2-space indentation, snake_case for method names, and CamelCase for class names, enforced by RuboCop (
.rubocop.yml)
Files:
app/queries/user_notifications_query.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/queries/user_notifications_query.rb
🧠 Learnings (9)
📓 Common learnings
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: 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: tyrrell-IH
Repo: fjordllc/bootcamp PR: 9306
File: app/javascript/components/Bookmarks.jsx:248-265
Timestamp: 2025-11-17T00:46:30.794Z
Learning: fjordllc/bootcamp プロジェクトでは、Reactからバニラ JavaScript への移行作業が進行中である。そのため、新しいReactファイルの作成は避け、既存のReactコンポーネント内で完結する実装が推奨される。BookmarksInDashboard.jsx は issue #9045 で削除予定。
Learnt from: Miya096jp
Repo: fjordllc/bootcamp PR: 9102
File: app/controllers/mentor/buzzes_controller.rb:0-0
Timestamp: 2025-09-04T01:39:22.261Z
Learning: fjordllc/bootcamp プロジェクトでは、更新処理の成功時の通知メッセージについて、I18n を使用せずに日本語文字列を直接記述する方針で一貫性を保っている。
Learnt from: reckyy
Repo: fjordllc/bootcamp PR: 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`が通知一覧のフィルタ対象ではないためで、意図的な設計判断である。
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:19-23
Timestamp: 2025-09-12T21:17:26.029Z
Learning: Rails アップグレードPRにおいて、config_for(:secrets) を使った 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のスコープ維持が重要。
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: sjabcdefin
Repo: fjordllc/bootcamp PR: 9090
File: app/javascript/user-sns.js:2-25
Timestamp: 2025-08-25T08:00:11.369Z
Learning: app/javascript/user-sns.js において、アクセシビリティ改善（aria-label属性の追加など）は他の箇所との整合性を保つため、別途統一して対応する方針である
📚 Learning: 2025-07-23T21:11:21.826Z
Learnt from: jun-kondo
Repo: fjordllc/bootcamp PR: 0
File: :0-0
Timestamp: 2025-07-23T21:11:21.826Z
Learning: fjordllc/bootcampプロジェクトの`app/models/inquiry_notifier.rb`において、`NOTIFICATION_SENDER_NAME`で指定される管理者ユーザーは必ず存在することがシステムの前提条件として保証されているため、senderがnilになることは想定されておらず、エラーハンドリングは不要である。
Applied to files:
app/views/notifications/index.html.slim
📚 Learning: 2025-07-15T12:58:02.735Z
Learnt from: reckyy
Repo: fjordllc/bootcamp PR: 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`が通知一覧のフィルタ対象ではないためで、意図的な設計判断である。
Applied to files:
app/views/notifications/index.html.slimapp/views/notifications/_filter_button.html.slimapp/queries/user_notifications_query.rb
📚 Learning: 2025-10-22T06:04:36.036Z
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状態の整合性を保証している。
Applied to files:
app/views/notifications/index.html.slim
📚 Learning: 2025-07-23T21:11:21.826Z
Learnt from: jun-kondo
Repo: fjordllc/bootcamp PR: 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オブジェクトは常に有効であることが保証されている。そのためエラーハンドリングは不要。
Applied to files:
app/views/notifications/index.html.slim
📚 Learning: 2025-09-12T21:17:26.029Z
Learnt from: komagata
Repo: fjordllc/bootcamp PR: 9101
File: app/notifiers/discord_notifier.rb:19-23
Timestamp: 2025-09-12T21:17:26.029Z
Learning: Rails アップグレードPRにおいて、config_for(:secrets) を使った webhook URL 設定の shared セクション未参照問題は、設定システム全体の変更として別PRで対応すべきである。
Applied to files:
app/views/notifications/index.html.slim
📚 Learning: 2025-11-17T00:46:30.794Z
Learnt from: tyrrell-IH
Repo: fjordllc/bootcamp PR: 9306
File: app/javascript/components/Bookmarks.jsx:248-265
Timestamp: 2025-11-17T00:46:30.794Z
Learning: fjordllc/bootcamp プロジェクトでは、Reactからバニラ JavaScript への移行作業が進行中である。そのため、新しいReactファイルの作成は避け、既存のReactコンポーネント内で完結する実装が推奨される。BookmarksInDashboard.jsx は issue #9045 で削除予定。
Applied to files:
app/views/notifications/index.html.slim
📚 Learning: 2025-08-28T00:34:28.541Z
Learnt from: mousu-a
Repo: fjordllc/bootcamp PR: 8566
File: app/views/application/_global_nav.slim:48-54
Timestamp: 2025-08-28T00:34:28.541Z
Learning: app/views/application/_global_nav.slim のQ&A/ペアワークバッジの環境分岐は、ペアワーク機能の本番リリース後に削除される一時的な実装である。その際、メンター・管理者のみに表示する制限仕様も撤廃される予定のため、現在の実装にはアクセス権限ガードを追加する必要がない。
Applied to files:
app/views/notifications/index.html.slimapp/views/notifications/_filter_button.html.slim
📚 Learning: 2025-08-25T08:00:11.369Z
Learnt from: sjabcdefin
Repo: fjordllc/bootcamp PR: 9090
File: app/javascript/user-sns.js:2-25
Timestamp: 2025-08-25T08:00:11.369Z
Learning: app/javascript/user-sns.js において、アクセシビリティ改善（aria-label属性の追加など）は他の箇所との整合性を保つため、別途統一して対応する方針である
Applied to files:
app/views/notifications/_filter_button.html.slim
⏰ 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/views/notifications/_filter_button.html.slim (1)
1-12: シンプルで意図が明確なフィルタ実装になっています
params[:target]を維持しつつ、statusだけを切り替える構造になっていて読みやすいです。params[:status]が'unread'以外の値でも「全て」タブがアクティブになるだけなので、UI 上も安全なフォールバックになっていると思います。app/views/notifications/index.html.slim (1)
47-68: 一覧ビューの分岐・ページネーション・メンター向けボタンの構成は問題なさそうです
- フィルタタブ → 空状態 (
@notifications.empty?) → 一覧＋ページネーション、という流れが整理されていて読みやすいです。- 空状態メッセージとメンター向け「未読の通知を一括で開く」ボタンの表示条件がどちらも
params[:status] == 'unread'に基づいていて、React 版の「未読タブ前提の機能」という前提もそのまま表現できていると思います。- ページネーションを上部・下部の両方に出す実装も既存 UI と整合していそうです。
特に修正必須な点は見当たらず、この方向性で良いと思います。
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/queries/user_notifications_query_test.rb (1)
15-16: アサーションスタイルの改善を検討できます（オプショナル）。現在の実装は正しく動作しますが、以下のようにより直接的な表現も可能です：
assert result.all? { |n| n.user == user }または：
assert_equal [user], result.map(&:user).uniq現在の実装でも問題ありませんので、必要に応じてご検討ください。
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/views/notifications/index.html.slim(1 hunks)test/queries/user_notifications_query_test.rb(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.slim
📄 CodeRabbit inference engine (AGENTS.md)
Slim templates should be linted according to
config/slim_lint.yml
Files:
app/views/notifications/index.html.slim
test/**/*_test.rb
📄 CodeRabbit inference engine (AGENTS.md)
test/**/*_test.rb: Test suite should use Minitest with structure:system/,models/,controllers/, and fixtures intest/fixtures/; test files should be named*_test.rb
Use Minitest + Capybara for system tests; place unit and integration tests under matchingtest/*directories
Files:
test/queries/user_notifications_query_test.rb
**/*.rb
📄 CodeRabbit inference engine (AGENTS.md)
Ruby code should use 2-space indentation, snake_case for method names, and CamelCase for class names, enforced by RuboCop (
.rubocop.yml)
Files:
test/queries/user_notifications_query_test.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/queries/user_notifications_query_test.rb
test/**
📄 CodeRabbit inference engine (AGENTS.md)
Keep tests deterministic and use fixtures stored in
test/fixtures/for test data
Files:
test/queries/user_notifications_query_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/queries/user_notifications_query_test.rb
🧠 Learnings (11)
📓 Common learnings
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: 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: tyrrell-IH
Repo: fjordllc/bootcamp PR: 9306
File: app/javascript/components/Bookmarks.jsx:248-265
Timestamp: 2025-11-17T00:46:30.794Z
Learning: fjordllc/bootcamp プロジェクトでは、Reactからバニラ JavaScript への移行作業が進行中である。そのため、新しいReactファイルの作成は避け、既存のReactコンポーネント内で完結する実装が推奨される。BookmarksInDashboard.jsx は issue #9045 で削除予定。
Learnt from: Miya096jp
Repo: fjordllc/bootcamp PR: 9102
File: app/controllers/mentor/buzzes_controller.rb:0-0
Timestamp: 2025-09-04T01:39:22.261Z
Learning: fjordllc/bootcamp プロジェクトでは、更新処理の成功時の通知メッセージについて、I18n を使用せずに日本語文字列を直接記述する方針で一貫性を保っている。
Learnt from: reckyy
Repo: fjordllc/bootcamp PR: 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`が通知一覧のフィルタ対象ではないためで、意図的な設計判断である。
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:52-56
Timestamp: 2025-09-12T21:17:45.295Z
Learning: Rails upgrade PRにおいて、configuration systemの改善提案（shared configuration fallback等）も、アップグレードに直接関連しない場合は別PRで対応すべきである。PRのスコープ維持が重要。
Learnt from: komagata
Repo: fjordllc/bootcamp PR: 9101
File: app/notifiers/discord_notifier.rb:19-23
Timestamp: 2025-09-12T21:17:26.029Z
Learning: Rails アップグレードPRにおいて、config_for(:secrets) を使った webhook URL 設定の shared セクション未参照問題は、設定システム全体の変更として別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: 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: 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やTimesChannelDestroyer等による重要なデータクリーンアップ処理を実行するため、管理者による退会（triggered_by: 'admin'）の場合でも必ず実行する必要がある。
Learnt from: sjabcdefin
Repo: fjordllc/bootcamp PR: 9090
File: app/javascript/user-sns.js:2-25
Timestamp: 2025-08-25T08:00:11.369Z
Learning: app/javascript/user-sns.js において、アクセシビリティ改善（aria-label属性の追加など）は他の箇所との整合性を保つため、別途統一して対応する方針である
📚 Learning: 2025-07-15T12:58:02.735Z
Learnt from: reckyy
Repo: fjordllc/bootcamp PR: 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`が通知一覧のフィルタ対象ではないためで、意図的な設計判断である。
Applied to files:
app/views/notifications/index.html.slimtest/queries/user_notifications_query_test.rb
📚 Learning: 2025-09-12T01:00:58.452Z
Learnt from: Miya096jp
Repo: fjordllc/bootcamp PR: 9102
File: app/views/mentor/buzzes/edit.html.slim:1-0
Timestamp: 2025-09-12T01:00:58.452Z
Learning: app/views/mentor/buzzes/edit.html.slim では `- title` と `- set_meta_tags` の両方が正しく設定されており、タイトルは削除されていない。変更内容はメタディスクリプションの追加のみ。
Applied to files:
app/views/notifications/index.html.slim
📚 Learning: 2025-08-28T00:34:28.541Z
Learnt from: mousu-a
Repo: fjordllc/bootcamp PR: 8566
File: app/views/application/_global_nav.slim:48-54
Timestamp: 2025-08-28T00:34:28.541Z
Learning: app/views/application/_global_nav.slim のQ&A/ペアワークバッジの環境分岐は、ペアワーク機能の本番リリース後に削除される一時的な実装である。その際、メンター・管理者のみに表示する制限仕様も撤廃される予定のため、現在の実装にはアクセス権限ガードを追加する必要がない。
Applied to files:
app/views/notifications/index.html.slim
📚 Learning: 2025-07-23T21:11:21.826Z
Learnt from: jun-kondo
Repo: fjordllc/bootcamp PR: 0
File: :0-0
Timestamp: 2025-07-23T21:11:21.826Z
Learning: fjordllc/bootcampプロジェクトの`app/models/inquiry_notifier.rb`において、`NOTIFICATION_SENDER_NAME`で指定される管理者ユーザーは必ず存在することがシステムの前提条件として保証されているため、senderがnilになることは想定されておらず、エラーハンドリングは不要である。
Applied to files:
app/views/notifications/index.html.slim
📚 Learning: 2025-10-22T06:04:36.036Z
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状態の整合性を保証している。
Applied to files:
app/views/notifications/index.html.slim
📚 Learning: 2025-07-23T21:11:21.826Z
Learnt from: jun-kondo
Repo: fjordllc/bootcamp PR: 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オブジェクトは常に有効であることが保証されている。そのためエラーハンドリングは不要。
Applied to files:
app/views/notifications/index.html.slim
📚 Learning: 2025-09-12T21:17:26.029Z
Learnt from: komagata
Repo: fjordllc/bootcamp PR: 9101
File: app/notifiers/discord_notifier.rb:19-23
Timestamp: 2025-09-12T21:17:26.029Z
Learning: Rails アップグレードPRにおいて、config_for(:secrets) を使った webhook URL 設定の shared セクション未参照問題は、設定システム全体の変更として別PRで対応すべきである。
Applied to files:
app/views/notifications/index.html.slim
📚 Learning: 2025-11-17T00:46:30.794Z
Learnt from: tyrrell-IH
Repo: fjordllc/bootcamp PR: 9306
File: app/javascript/components/Bookmarks.jsx:248-265
Timestamp: 2025-11-17T00:46:30.794Z
Learning: fjordllc/bootcamp プロジェクトでは、Reactからバニラ JavaScript への移行作業が進行中である。そのため、新しいReactファイルの作成は避け、既存のReactコンポーネント内で完結する実装が推奨される。BookmarksInDashboard.jsx は issue #9045 で削除予定。
Applied to files:
app/views/notifications/index.html.slim
📚 Learning: 2025-11-26T08:23:30.927Z
Learnt from: CR
Repo: fjordllc/bootcamp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T08:23:30.927Z
Learning: Applies to test/**/*_test.rb : Test suite should use Minitest with structure: `system/`, `models/`, `controllers/`, and fixtures in `test/fixtures/`; test files should be named `*_test.rb`
Applied to files:
test/queries/user_notifications_query_test.rb
📚 Learning: 2025-11-26T08:23:30.927Z
Learnt from: CR
Repo: fjordllc/bootcamp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T08:23:30.927Z
Learning: Applies to test/**/*_test.rb : Use Minitest + Capybara for system tests; place unit and integration tests under matching `test/*` directories
Applied to files:
test/queries/user_notifications_query_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 (2)
app/views/notifications/index.html.slim (1)
47-68: 実装が適切です。過去の指摘事項も対応済みです。以下の点が適切に実装されています：
- 空の状態メッセージが
params[:status]に応じて適切に表示されている- ページネーションが
total_pages > 1の場合のみ表示される- 各通知がパーシャル経由で描画されている
- メンター専用の一括開封アクションが条件付きで表示されている
過去のレビューで指摘された
hrタグのインデントエラーは commit ef048e2 で修正済み、params[:status]のバリデーションについてもモデル側で安全に処理されていることが確認されています。test/queries/user_notifications_query_test.rb (1)
5-44: テストの実装が適切です。ファイル名の問題も解決済みです。3つのテストケースがそれぞれ異なる観点（ユーザーフィルタリング、ステータスフィルタリング、ターゲットフィルタリング）をカバーしており、基本的なテストカバレッジとして十分です。
過去のレビューで指摘されたファイル名の問題（
_test.rbサフィックスが必要）は、現在のファイル名user_notifications_query_test.rbで正しく解決されています。Rails のテスト自動検出パターンに合致しており、bin/rails testで正常に実行されます。
- ファイル名を`_test.rb`に修正 - テストが通るように変更(ページネーションの箇所,URLのクエリ順番)
smallmonkeykey
force-pushed
the
chore/replace-notifications-to-rails-view
branch
from
21ff85e to
07803f2
Compare
|
@jun-kondo また、rebase -iに関してとても丁寧に教えてくださってありがとうございます😭 ❣️ |
smallmonkeykey
changed the title
|
@smallmonkeykey |
|
@jun-kondo |
|
@smallmonkeykey 問題ないと思いますので、approve致します🙋♂️ |
|
@jun-kondo @komagata |
5 participants
Issue
概要
脱React化の一環として、通知機能一覧の箇所をReactからRails viewとJSに移行しました。
下記の二つのjsxファイルをプロジェクトから削除しました。
変更確認方法
komagataでログインしてください全ての未読通知一覧へを押して飛ぶか、http://localhost:3000/notifications?status=unreadに直接飛んでくださいhttp://localhost:3000/notifications?status=unreadには未読の通知があることを確認してください全てをクリックし、http://localhost:3000/notificationsには全ての通知があることを確認してください未読の未確認通知を一括で開くボタンがあることを確認してくださいhttp://localhost:3000/notifications?status=unreadに戻ると、さきほど開いた通知がなくなっていることを確認してください全てをクリックし、さきほど開いた通知が移動していることを確認してくださいhttp://localhost:3000/notifications?status=unreadに飛び、未読の未確認通知を一括で開くボタンがないことを確認してくださいScreenshot
見た目の変化はないためスクリーンショットはありません。
ブラウザバック時の動作の違い
React版とRails版では、通知一覧ページの未読から詳細ページに遷移したあと、ブラウザバックする時の動作が異なります。
React版
ブラウザバックすると瞬時に消えている
Rails版
ブラウザバックすると未読のままとして残っている
Railsは、ブラウザバックを行った際に、サーバーから新しくページを取得せずにブラウザがキャッシュしていたHTMLを再表示するため、削除前の通知が一時的に残って見えることがあります。
今回はこの挙動を仕組み上の違いによる仕様として扱い、
Rails 版ではブラウザバック時に一時的に古い通知が表示される場合がある状態を許容しています。
再読み込みを行うことで、サーバー上の最新状態が正しく反映されます。
11月19日のMTGにてこちらの挙動を伝えており、今回はこちらの方針で大丈夫とのことです。
Summary by CodeRabbit
リファクタリング
新機能
テスト
✏️ Tip: You can customize this high-level summary in your review settings.