Conversation
Walkthroughブックマーク一覧をReactからサーバー描画へ移行。コントローラで@bookmarksを事前読み込み（1ページ20件）、destroyアクションとルートを追加。クライアントJSは部分再描画と削除/表示切替を担い、関連ビュー・テスト・パックを更新。 Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as ユーザ
participant B as ブラウザ (bookmarks.js)
participant C as CurrentUser::BookmarksController
participant V as サーバー (views .page-main)
U->>B: 編集トグルをON/OFF
B->>B: toggleDeleteButtonVisibility適用（表示/非表示）
U->>B: 削除ボタンをクリック
B->>C: DELETE /current_user/bookmarks/:id
C-->>B: 204 No Content
B->>C: GET /current_user/bookmarks?page=<現在ページ>
C->>V: render index -> (_list or _empty)
C-->>B: HTML (.page-main)
alt 現在ページが空 かつ page>1
B->>C: GET /current_user/bookmarks?page=<現在ページ-1>
C-->>B: HTML (.page-main)
end
B->>B: .page-main/.page-body を置換して初期化（編集モード維持）
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 注意して確認すべき箇所:
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (7)
💤 Files with no reviewable changes (2)
🚧 Files skipped from review as they are similar to previous changes (5)
⏰ 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)
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: 2
🧹 Nitpick comments (14)
test/system/bookmark/talks_test.rb (2)
14-14: 条件付きassertはテストをすり抜ける可能性あり1ページ目に対象テキストがない場合にスキップされ、検証されません。不要なら削除、もしくは後段のページ遷移後にのみ検証する形に寄せましょう。
- assert_text "#{@decorated_user.long_name} さんの相談部屋" if page.has_text?(@decorated_user.long_name) -
16-16: ページネーション依存の前提を明示してからクリックリンクが存在しない環境での不安定化を避けるため、存在確認を入れてからクリックしてください。
+ assert_selector 'a.pagination__item-link', text: '2' find('a.pagination__item-link', text: '2').clickapp/views/current_user/bookmarks/index.html.slim (1)
7-7: title/metaの削除はSEO・一貫性に影響する可能性従来のページタイトル/descriptionがなくなるため、必要なら復活を検討してください。
+ - title 'ブックマーク一覧' + - set_meta_tags description: 'ブックマーク一覧ページです。' = render 'home/page_header' = dashboard_page_tabs(active_tab: 'ブックマーク') = render 'current_user/bookmarks/list', bookmarks: @bookmarksconfig/routes/current_user.rb (1)
8-8: 微修正: 余計なスペースの除去不要な二重スペースを削ると差分が綺麗になります。
- resources :bookmarks, only: %i(index destroy) + resources :bookmarks, only: %i(index destroy)app/javascript/bookmarks-delete-button.js (1)
11-23: fetchではなく@rails/request.jsを使用してください（ガイドライン準拠）プロジェクト方針に従いrequest.jsへ置換しましょう。CSRFも自動処理され、記述量も減ります。
- fetch(url, { - method: 'DELETE', - headers: { - 'X-CSRF-Token': document.querySelector('meta[name="csrf-token"]') - .content, - Accept: 'application/json' - } - }) - .then((response) => response.text()) - .then((html) => { - const pageMain = document.querySelector('.page-main') - if (pageMain) pageMain.outerHTML = html - }) - .catch((error) => console.error('削除に失敗しました', error)) + import('@rails/request.js').then(async ({ FetchRequest }) => { + try { + const request = new FetchRequest('delete', url, { + headers: { Accept: 'text/html' } + }) + const response = await request.perform() + if (response.ok) { + const html = await response.text() + const pageMain = document.querySelector('.page-main') + if (pageMain) pageMain.outerHTML = html + } else { + console.error('削除に失敗しました', response.status, response.statusText) + } + } catch (error) { + console.error('削除に失敗しました', error) + } + })app/controllers/current_user/bookmarks_controller.rb (3)
8-9: N+1クエリ回避のためにuserを事前読込ビューで
bookmark.bookmarkable.userを参照しているため、includes(bookmarkable: :user)にしてください。- @bookmarks = current_user.bookmarks.includes(:bookmarkable).order(created_at: :desc, id: :desc).page(params[:page]).per(PAGER_NUMBER) + @bookmarks = current_user.bookmarks.includes(bookmarkable: :user).order(created_at: :desc, id: :desc).page(params[:page]).per(PAGER_NUMBER) @@ - @bookmarks = current_user.bookmarks.includes(:bookmarkable).order(created_at: :desc, id: :desc).page(params[:page]).per(PAGER_NUMBER) + @bookmarks = current_user.bookmarks.includes(bookmarkable: :user).order(created_at: :desc, id: :desc).page(params[:page]).per(PAGER_NUMBER)Also applies to: 13-13
4-4: ページサイズ定数の分散管理を避けるAPI側(PAGER_NUMBER=25)と数値が乖離しています。共通化（設定 or 共通モジュール）を検討してください。
7-7: 未使用インスタンス変数の削除
@user = current_userは本ビューでは未使用に見えます。削除してください。- @user = current_userapp/views/current_user/bookmarks/_list.html.slim (6)
1-1: 部分テンプレでインスタンス変数参照は避け、localsに統一再利用性・可読性のため、
@bookmarksではなくbookmarksローカル変数を使いましょう。-- if @bookmarks.empty? +- if bookmarks.empty? @@ - - if @bookmarks.total_pages > 1 - = paginate @bookmarks + - if bookmarks.total_pages > 1 + = paginate bookmarks @@ - - @bookmarks.each do |bookmark| + - bookmarks.each do |bookmark|Also applies to: 35-36, 40-40
41-41: DOM idはRailsヘルパを利用して一意性と一貫性を担保
dom_idを使うとテスト・CSSの保守性が上がります。- .card-list-item class="is-#{bookmark.bookmarkable_type.downcase}" id="bookmark-#{bookmark.id}" + .card-list-item class="is-#{bookmark.bookmarkable_type.downcase}" id=dom_id(bookmark)
46-46: i18nはmodel_name.humanを利用人名化は
model_name.humanが簡潔で辞書の探索も安全です。- .card-list-item__label = t("activerecord.models.#{bookmark.bookmarkable.class.to_s.tableize.singularize}") + .card-list-item__label = bookmark.bookmarkable.model_name.human
52-53: URLではなくpathを使用（相対パス化）アプリ内遷移は
polymorphic_pathが適切です。外部公開URLが必要な場面以外は*_urlは不要です。- = link_to polymorphic_url(bookmark.bookmarkable), class: 'card-list-item-title__link a-text-link' do + = link_to polymorphic_path(bookmark.bookmarkable), class: 'card-list-item-title__link a-text-link' do
40-40: 安全性: orphanレコード対策
bookmark.bookmarkableがnil（元データ削除済み）の場合に例外になります。スキップ or フォールバック表示を追加してください。- - bookmarks.each do |bookmark| + - bookmarks.each do |bookmark| + - next unless bookmark.bookmarkable.present?
35-37: 任意: paginateの条件分岐は不要kaminariは1ページの場合は何も描画しないため、
if total_pages > 1の条件は省略可能です（好みの問題）。
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
app/controllers/current_user/bookmarks_controller.rb(1 hunks)app/javascript/bookmarks-delete-button.js(1 hunks)app/javascript/components/Bookmarks.jsx(0 hunks)app/javascript/packs/application.js(1 hunks)app/views/current_user/bookmarks/_list.html.slim(1 hunks)app/views/current_user/bookmarks/index.html.slim(1 hunks)config/routes/current_user.rb(1 hunks)test/system/bookmark/talks_test.rb(1 hunks)
💤 Files with no reviewable changes (1)
- app/javascript/components/Bookmarks.jsx
🧰 Additional context used
📓 Path-based instructions (3)
**/*.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/current_user/bookmarks_controller.rbtest/system/bookmark/talks_test.rbconfig/routes/current_user.rb
**/*.js
⚙️ CodeRabbit configuration file
**/*.js: - どうしても避けられない時以外に新規にVue.js, Reactのコードを追加しない。
- fetchメソッドの代わりにrequest.jsを使う。
Files:
app/javascript/bookmarks-delete-button.jsapp/javascript/packs/application.js
test/**/*
⚙️ CodeRabbit configuration file
test/**/*: # Test
- TestCase名は英語で書く。
- どうしても避けられない時以外にsystem testでsleepは使わない。
Unit Test
model, helper, decorator, view_componentについてはメソッドを追加した場合は必ず対応したUnit TestのTestCaseを1つは書く。
Files:
test/system/bookmark/talks_test.rb
🧠 Learnings (1)
📓 Common learnings
Learnt from: komagata
PR: fjordllc/bootcamp#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で対応すべきである。
🧬 Code graph analysis (2)
app/controllers/current_user/bookmarks_controller.rb (1)
app/controllers/api/bookmarks_controller.rb (3)
index(3-41)index(6-17)destroy(31-34)
test/system/bookmark/talks_test.rb (1)
app/decorators/user_decorator.rb (1)
long_name(40-42)
⏰ 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/javascript/packs/application.js (1)
46-46: ブックマーク削除スクリプトの読込追加は妥当ですイベント委譲でのクリックハンドリング前提ならimport位置も問題ありません。
config/routes/current_user.rb (1)
8-8: destroyの追加OK（RESTful化）現状の要件に合致しています。
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 (5)
app/controllers/api/bookmarks_controller.rb (5)
13-16: 未完成コード（構文エラー）: bookmarkable が未指定のままこのままだと SyntaxError になります。bookmarkable フィルタを完成させてください。ついでに安定ソートと includes を揃えましょう。
- @bookmarks = Bookmark.where( - user: current_user, - bookmarkable: - ) + scope = Bookmark.where(user: current_user) + .includes(bookmarkable: :user) + .order(created_at: :desc, id: :desc) + record = bookmarkable + return head :bad_request unless record + @bookmarks = scope.where(bookmarkable: record) + .page(params[:page]) + .per(per)
19-29: create が未完成＋重複判定の誤り（type 未考慮）
bookmarkable:が未指定のままです（構文/機能バグ）。exists?がbookmarkable_idのみなので polymorphic で type が衝突します。- 競合にも弱いので
find_or_create_byかユニーク制約の併用を。- def create - if Bookmark.exists?(user_id: current_user.id, bookmarkable_id: params[:bookmarkable_id]) - head :no_content - else - @bookmark = Bookmark.create!( - user: current_user, - bookmarkable: - ) - render status: :created, json: @bookmark - end - end + def create + record = bookmarkable + return head :bad_request unless record + + @bookmark = Bookmark.find_or_create_by!(user: current_user, bookmarkable: record) + render status: :created, json: @bookmark + rescue ActiveRecord::RecordNotUnique + # DBユニーク制約併用時の競合リトライ + head :no_content + end
31-34: 権限漏れ: 他ユーザーのブックマークを削除可能
Bookmark.findが current_user でスコープされていません。越権削除が可能です。- Bookmark.find(params[:id]).destroy + current_user.bookmarks.find(params[:id]).destroy head :no_content
7-10: paginate_array による全件ロードでパフォーマンス悪化ActiveRecord::Relation を
paginate_arrayに包むと全件メモリ展開されます。素直に Relation のままページングしてください。併せて tie-breaker を入れて安定ソートに。- per = params[:per] || PAGER_NUMBER - bookmarks = Bookmark.where(user: current_user).order(created_at: :desc).preload(bookmarkable: :user) - @bookmarks = Kaminari.paginate_array(bookmarks).page(params[:page]).per(per) - @unpaged_bookmarks = bookmarks + per = (params[:per] || PAGER_NUMBER).to_i.clamp(1, 100) + scope = Bookmark.where(user: current_user) + .includes(bookmarkable: :user) + .order(created_at: :desc, id: :desc) + @bookmarks = scope.page(params[:page]).per(per) + @unpaged_bookmarks = scope
38-40: 任意の constantize は危険（DoS/想定外クラス）ユーザー入力を
constantizeするのは避け、許可リスト＋safe_constantizeを使ってください。未知 type は 400/404 を返すのが無難です。- def bookmarkable - params[:bookmarkable_type].constantize.find_by(id: params[:bookmarkable_id]) - end + ALLOWED_BOOKMARKABLES = { + # 'ModelName' => ModelName, + }.freeze + + def bookmarkable + type_name = params[:bookmarkable_type].to_s + klass = ALLOWED_BOOKMARKABLES[type_name] || type_name.safe_constantize + return nil unless klass&.respond_to?(:find_by) + klass.find_by(id: params[:bookmarkable_id]) + end許可対象はプロジェクトの実体に合わせて追記してください。
🧹 Nitpick comments (6)
app/controllers/current_user/bookmarks_controller.rb (2)
10-17: HTML を返すならクライアントで適用する／返さないなら 204 に統一現状、サーバは部分テンプレート(HTML)を返しますが、JS 側は本文を使っていません。帯域節約のため 204 を返すか、返した HTML を適用するか、どちらかに揃えましょう。
— サーバを 204 にする案:
def destroy current_user.bookmarks.find(params[:id]).destroy - @bookmarks = current_user.bookmarks.includes(bookmarkable: :user).order(created_at: :desc, id: :desc).page(params[:page]).per(PAGER_NUMBER) - - render partial: 'current_user/bookmarks/list', - formats: [:html], - locals: { bookmarks: @bookmarks } + head :no_content end— 返した HTML を適用する案（JS を併修正し
.innerHTML = await response.text()などで置換）:def destroy current_user.bookmarks.find(params[:id]).destroy @bookmarks = current_user.bookmarks.includes(bookmarkable: :user).order(created_at: :desc, id: :desc).page(params[:page]).per(PAGER_NUMBER) render partial: 'current_user/bookmarks/list', formats: [:html], locals: { bookmarks: @bookmarks } endどちらで進めるか確認お願いします。
4-4: PAGER_NUMBER の重複定義（API と二重管理）API 側にも同名定数があります。値の乖離を防ぐため、Kaminari 設定を使う or 共有モジュール化をご検討ください。
app/javascript/bookmarks-delete-button.js (4)
21-23:response.errorは存在しません — エラー文が常に undefinedFetch/Request.js の
Responseにerrorはありません。status 等で明確なメッセージを出してください。- if (!response.ok) { - throw new Error(`${response.error}`) - } + if (!response.ok) { + const body = await response.text().catch(() => '') + throw new Error(`DELETE ${url} failed: ${response.status} ${response.statusText}${body ? ` — ${body}` : ''}`) + }
17-19: Request.js の応答種別を明示（任意）HTML を受ける意図が明確になるよう
responseKind: 'html'を付けても良いです。- const response = await destroy(url, { - headers: { Accept: 'text/html' } - }) + const response = await destroy(url, { + responseKind: 'html', + headers: { Accept: 'text/html' } + })
15-16: ガード追加: data-url 未設定時の安全退出予期せぬマークアップ時の例外を回避します。
const url = deleteButton.dataset.url + if (!url) { + console.warn('Delete URL is missing on .bookmark-delete-button') + return + }
25-43: DOM 操作の軽微な整理（再利用と無駄なクエリ削減）対象要素を変数に保持して二度検索を避けると読みやすくなります。
- const cardListItems = document.querySelectorAll('.card-list-item') - - if (cardListItems.length === 1) { - deleteButton.closest('.card-list-item').remove() + const items = document.querySelectorAll('.card-list-item') + const item = deleteButton.closest('.card-list-item') + if (items.length === 1) { + item?.remove() const pageBody = document.querySelector('.page-body') if (pageBody) { pageBody.innerHTML = ` @@ - } else { - deleteButton.closest('.card-list-item').remove() - } + } else { + item?.remove() + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/controllers/api/bookmarks_controller.rb(1 hunks)app/controllers/current_user/bookmarks_controller.rb(1 hunks)app/javascript/bookmarks-delete-button.js(1 hunks)app/views/current_user/bookmarks/_list.html.slim(1 hunks)config/routes/current_user.rb(1 hunks)test/system/bookmark/talks_test.rb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- config/routes/current_user.rb
- test/system/bookmark/talks_test.rb
- app/views/current_user/bookmarks/_list.html.slim
🧰 Additional context used
📓 Path-based instructions (2)
**/*.js
⚙️ CodeRabbit configuration file
**/*.js: - どうしても避けられない時以外に新規にVue.js, Reactのコードを追加しない。
- fetchメソッドの代わりにrequest.jsを使う。
Files:
app/javascript/bookmarks-delete-button.js
**/*.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/api/bookmarks_controller.rbapp/controllers/current_user/bookmarks_controller.rb
🧠 Learnings (1)
📚 Learning: 2025-09-04T01:39:22.261Z
Learnt from: Miya096jp
PR: fjordllc/bootcamp#9102
File: app/controllers/mentor/buzzes_controller.rb:0-0
Timestamp: 2025-09-04T01:39:22.261Z
Learning: fjordllc/bootcamp プロジェクトでは、更新処理の成功時の通知メッセージについて、I18n を使用せずに日本語文字列を直接記述する方針で一貫性を保っている。
Applied to files:
app/controllers/current_user/bookmarks_controller.rb
🧬 Code graph analysis (2)
app/javascript/bookmarks-delete-button.js (2)
app/controllers/api/bookmarks_controller.rb (1)
destroy(31-34)app/controllers/current_user/bookmarks_controller.rb (1)
destroy(10-17)
app/controllers/current_user/bookmarks_controller.rb (1)
app/controllers/api/bookmarks_controller.rb (3)
index(3-41)index(6-17)destroy(31-34)
⏰ 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/current_user/bookmarks_controller.rb (1)
7-8: 一覧の includes + 安定ソート + ページングは適切ですN+1 回避と安定した並び順で問題ありません。
app/controllers/api/bookmarks_controller.rb (1)
4-4: API のデフォルト件数を 20 に変更 — 25 件前提の依存を確認外部クライアントやコード内で 25 件前提の箇所がないか確認（回帰防止）。対象: app/controllers/api/bookmarks_controller.rb の PAGER_NUMBER = 20（行 4）。リポジトリ内を検索するには下記を実行し、結果を報告。
git grep -n -E 'per[[:space:]]*[:=][[:space:]]*25|per([[:space:]]25[[:space:]])|PAGER_NUMBER' || true
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
app/javascript/bookmarks-delete-button.js (3)
18-18: 開発環境でのコンソール出力を削除してください本番環境でレスポンスオブジェクトをコンソールに出力するのは適切ではありません。デバッグ用のログは削除するか、開発環境のみに限定してください。
- console.log(response)または、開発環境のみに制限する場合：
- console.log(response) + if (process.env.NODE_ENV === 'development') { + console.log(response) + }
20-22: エラーメッセージにHTTPステータスコードを含めることを検討してください削除失敗時のデバッグを容易にするため、HTTPステータスコードをエラーメッセージに含めることをお勧めします。
if (!response.ok) { - throw new Error(`削除に失敗しました。`) + throw new Error(`削除に失敗しました。(ステータス: ${response.status})`) }
27-27: エラーハンドリングの改善を検討してください
console.warnだけでは、ユーザーにエラーが伝わりません。ユーザーへのフィードバックを追加することを検討してください。} catch (error) { console.warn(error) + alert('削除処理中にエラーが発生しました。ページを再読み込みしてください。') }または、より洗練されたUI通知システムがあれば、それを使用してください。
app/views/current_user/bookmarks/_list.html.slim (4)
30-30: チェックボックスのIDが重複する可能性があります
#bookmark_editのIDが空の状態（22行目付近でも同様のIDが必要な可能性）と非空の状態の両方で使用される場合、ID重複の問題が発生する可能性があります。現在のコードでは条件分岐により問題ありませんが、将来的な拡張を考慮してクラスベースのセレクタを使用することを検討してください。- input#bookmark_edit[name="card-list-tools__action" type="checkbox"] + input.bookmark-edit-toggle#bookmark_edit[name="card-list-tools__action" type="checkbox"]
52-53: リンクのインデントが不正です52行目の
link_toのインデントが1スペース多くなっています。.card-list-item-title__title - = link_to polymorphic_path(bookmark.bookmarkable), class: 'card-list-item-title__link a-text-link' do + = link_to polymorphic_path(bookmark.bookmarkable), class: 'card-list-item-title__link a-text-link' do
73-73: 削除ボタンのhrefを改善することを検討してください
href="#"は、クリック時にページの先頭にスクロールする可能性があります。JavaScriptで制御されているとはいえ、より適切な値を使用することをお勧めします。- = link_to '削除', '#', class: 'bookmark-delete-button a-bookmark-button a-button is-sm is-block is-main', data: { url: current_user_bookmark_path(bookmark) } + = link_to '削除', 'javascript:void(0)', class: 'bookmark-delete-button a-bookmark-button a-button is-sm is-block is-main', data: { url: current_user_bookmark_path(bookmark) }または、
button要素の使用を検討してください：- = link_to '削除', '#', class: 'bookmark-delete-button a-bookmark-button a-button is-sm is-block is-main', data: { url: current_user_bookmark_path(bookmark) } + button.bookmark-delete-button.a-bookmark-button.a-button.is-sm.is-block.is-main[type="button" data-url=current_user_bookmark_path(bookmark)] 削除
67-67: reported_onの存在確認を追加することを検討してください
Reportタイプのブックマークでreported_onがnilの場合、エラーが発生する可能性があります。- - date = bookmark.bookmarkable_type == 'Report' ? bookmark.bookmarkable.reported_on.to_time : bookmark.bookmarkable.created_at + - date = bookmark.bookmarkable_type == 'Report' && bookmark.bookmarkable.reported_on ? bookmark.bookmarkable.reported_on.to_time : bookmark.bookmarkable.created_at
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/controllers/current_user/bookmarks_controller.rb(1 hunks)app/javascript/bookmarks-delete-button.js(1 hunks)app/views/current_user/bookmarks/_list.html.slim(1 hunks)test/system/current_user/bookmarks_test.rb(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/controllers/current_user/bookmarks_controller.rb
🧰 Additional context used
📓 Path-based instructions (3)
**/*.js
⚙️ CodeRabbit configuration file
**/*.js: - どうしても避けられない時以外に新規にVue.js, Reactのコードを追加しない。
- fetchメソッドの代わりにrequest.jsを使う。
Files:
app/javascript/bookmarks-delete-button.js
**/*.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/current_user/bookmarks_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/current_user/bookmarks_test.rb
🧠 Learnings (1)
📓 Common learnings
Learnt from: komagata
PR: fjordllc/bootcamp#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で対応すべきである。
🧬 Code graph analysis (1)
app/javascript/bookmarks-delete-button.js (1)
app/controllers/current_user/bookmarks_controller.rb (1)
destroy(10-14)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build_and_test
🔇 Additional comments (4)
test/system/current_user/bookmarks_test.rb (3)
29-29: 良い変更です！セレクタを
input#bookmark_editに更新したのは、新しいRailsビューの実装と一致していて適切です。
100-102: 良いテストの更新です！
.page-mainコンテナ内でページネーションをチェックするように変更したのは、新しいサーバーサイドレンダリングの構造と一致していて適切です。
122-124: 一貫性のあるDOM構造の検証21件以上のブックマークがある場合のページネーションテストも、
.page-mainコンテナを使用するよう適切に更新されています。app/javascript/bookmarks-delete-button.js (1)
24-24: response.textは関数呼び出しが必要です
response.textはメソッドなので、括弧を付けて呼び出す必要があります。現在のコードでは、関数オブジェクト自体を取得してしまい、期待通り動作しません。- const html = await response.text + const html = await response.text()⛔ Skipped due to learnings
Learnt from: Miya096jp PR: fjordllc/bootcamp#8877 File: app/javascript/watches.js:78-78 Timestamp: 2025-07-26T01:11:03.921Z Learning: Rails request.jsのFetchResponseクラスでは、text、json、htmlプロパティはgetterとして定義されており、response.text()ではなくresponse.textとしてアクセスする必要がある。getterは自動的にレスポンスをキャッシュし、jsonの場合はContent-Typeの検証も行う。Learnt from: Miya096jp PR: fjordllc/bootcamp#0 File: :0-0 Timestamp: 2025-07-04T07:17:55.949Z Learning: Rails request.jsのFetchResponseクラスでは、json、text、htmlなどはgetterとして定義されており、response.json()ではなくresponse.jsonとしてアクセスする必要がある。また、getterはContent-Typeの自動チェック機能を含んでいる。
smallmonkeykey
force-pushed
the
chore/migrate-bookmarks-to-rails-view
branch
from
d981703 to
7dd09ba
Compare
smallmonkeykey
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
app/controllers/api/bookmarks_controller.rb(1 hunks)app/controllers/current_user/bookmarks_controller.rb(1 hunks)app/javascript/bookmarks-delete-button.js(1 hunks)app/javascript/components/Bookmarks.jsx(0 hunks)app/javascript/packs/application.js(1 hunks)app/views/current_user/bookmarks/_list.html.slim(1 hunks)app/views/current_user/bookmarks/index.html.slim(1 hunks)config/routes/current_user.rb(1 hunks)test/system/bookmark/talks_test.rb(1 hunks)test/system/current_user/bookmarks_test.rb(3 hunks)
💤 Files with no reviewable changes (1)
- app/javascript/components/Bookmarks.jsx
🚧 Files skipped from review as they are similar to previous changes (8)
- test/system/current_user/bookmarks_test.rb
- app/controllers/api/bookmarks_controller.rb
- app/controllers/current_user/bookmarks_controller.rb
- config/routes/current_user.rb
- app/views/current_user/bookmarks/index.html.slim
- test/system/bookmark/talks_test.rb
- app/javascript/bookmarks-delete-button.js
- app/javascript/packs/application.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 (8)
app/views/current_user/bookmarks/_list.html.slim (8)
1-14: 空状態の表示が適切に実装されているブックマークが空の場合の表示が適切に実装されています。UIは一貫性があり、悲しい顔のアイコンと分かりやすいメッセージで空状態を表現しています。
35-36: ページネーションの重複表示ページネーションが上下両方に表示される設計は理にかなっています。長いリストでもユーザーがスクロールせずにナビゲートできます。
Also applies to: 75-76
41-41: 動的CSSクラスとID生成の実装動的なCSSクラス（
is-#{bookmark.bookmarkable_type.downcase}）とID（bookmark-#{bookmark.id}）の生成が適切に実装されています。これにより各ブックマークタイプに応じたスタイリングと個別の識別が可能になります。
43-46: Talkタイプの特別処理Talkタイプのブックマークに対してアバター画像を表示し、その他のタイプにはモデル名のラベルを表示する条件分岐が適切に実装されています。
52-53: polymorphic_pathの使用とタイトル表示
polymorphic_pathを使用した動的なリンク生成と、Talkタイプに対する特別なタイトル表示（「○○さんの相談部屋」）が適切に実装されています。
67-69: 日付処理の条件分岐Reportタイプの場合は
reported_onを、その他の場合はcreated_atを使用する条件分岐が適切に実装されています。日付のローカライゼーションも適切に処理されています。
58-58: search_summaryヘルパーは定義済み — 修正不要app/helpers/search_helper.rb の 35行目に def search_summary(resource, keyword) が存在します。
25-31: 編集モードのトグル実装を検証 — 削除ボタン制御は確認済み、IDの一貫性を要確認
- 確認: app/javascript/bookmarks-edit-button.js が document.getElementById('bookmark_edit') を監視し、class "js-bookmark-delete-button" を取得して削除ボタンの表示/非表示を制御している（app/javascript/bookmarks-edit-button.js）。packs/application.js で読み込まれている。
- 別箇所: app/javascript/watches.js と app/javascript/components/BookmarksInDashboard.jsx は ID "card-list-tools__action" を参照している（app/javascript/watches.js:6、BookmarksInDashboard.jsx:68-74）。
- 問題: 対象ビュー (app/views/current_user/bookmarks/_list.html.slim:25-31) では input の id が "bookmark_edit"、name が "card-list-tools__action" になっているため、watches.js が getElementById('card-list-tools__action') を期待すると要素を見つけられず動作しない可能性がある。
- 対応指示: ID を統一する（例: input に id="card-list-tools__action" を追加、または watches.js を 'bookmark_edit' を参照するよう修正）または両者が別コンテキストであることを確認して修正不要と判断する。対象ファイル: app/views/current_user/bookmarks/_list.html.slim、app/javascript/bookmarks-edit-button.js、app/javascript/watches.js、app/javascript/components/BookmarksInDashboard.jsx。
| .js-bookmark-delete-button | ||
| button.bookmark-delete-button.a-bookmark-button.a-button.is-sm.is-block.is-main[type="button" data-url=current_user_bookmark_path(bookmark)] 削除 |
There was a problem hiding this comment.
🧩 Analysis chain
削除ボタンの実装とJavaScript連携を確認
削除ボタンにdata-url属性で削除エンドポイントが設定され、適切なCSSクラスが適用されています。対応するJavaScriptファイル（bookmarks-delete-button.js）が適切に実装されていることを確認してください。
以下のスクリプトで削除ボタンを制御するJavaScriptコードを確認してください：
🏁 Script executed:
#!/bin/bash
# Description: ブックマーク削除機能のJavaScript実装を確認
# bookmarks-delete-button.jsファイルの内容を確認
cat app/javascript/bookmarks-delete-button.js
# 削除ボタンに関するJavaScriptコードを検索
rg -n --type=js -C3 "bookmark.*delete|js-bookmark-delete-button" app/javascript/Length of output: 3060
削除処理が動作しない — response.text() の呼び出し漏れ
app/javascript/bookmarks-delete-button.js を確認しました。destroy() 成功後のレスポンス取得で
const html = await response.text
となっており、正しくは
const html = await response.text()
です。これを修正してください。data-url 属性と application.js の import は問題ありません。
🤖 Prompt for AI Agents
In app/views/current_user/bookmarks/_list.html.slim around lines 72 to 73, the
delete button markup is fine but the delete handler in
app/javascript/bookmarks-delete-button.js misses calling response.text(); update
the destroy success path to await response.text() (i.e., call response.text()
with parentheses) when reading the response body, then proceed to use the
returned HTML as before; data-url attribute and application.js import require no
changes.
There was a problem hiding this comment.
rails/request.jsを使っているので、text()ではなくtextだと思うのですがどうですか？👀
There was a problem hiding this comment.
@smallmonkeykey さん、おっしゃる通りです！申し訳ありません。
Rails request.jsのFetchResponseクラスでは、text、json、htmlはgetterとして定義されており、response.text()ではなくresponse.textとしてアクセスする必要があります。現在のコードのconst html = await response.textは正しい実装です。
私の前回のコメントは間違いでした。標準のFetch APIの使い方と混同してしまいました。コードに問題はありません。
ご指摘いただき、ありがとうございます！🙏
🧠 Learnings used
Learnt from: Miya096jp
PR: fjordllc/bootcamp#8877
File: app/javascript/watches.js:78-78
Timestamp: 2025-07-26T01:11:03.921Z
Learning: Rails request.jsのFetchResponseクラスでは、text、json、htmlプロパティはgetterとして定義されており、response.text()ではなくresponse.textとしてアクセスする必要がある。getterは自動的にレスポンスをキャッシュし、jsonの場合はContent-Typeの検証も行う。
Learnt from: Miya096jp
PR: fjordllc/bootcamp#0
File: :0-0
Timestamp: 2025-07-04T07:17:55.949Z
Learning: Rails request.jsのFetchResponseクラスでは、json、text、htmlなどはgetterとして定義されており、response.json()ではなくresponse.jsonとしてアクセスする必要がある。また、getterはContent-Typeの自動チェック機能を含んでいる。
Learnt from: Miya096jp
PR: fjordllc/bootcamp#0
File: :0-0
Timestamp: 2025-07-04T07:17:55.949Z
Learning: Rails request.jsのFetchResponseクラスでは、jsonとtextはgetterとして定義されており、それぞれPromiseを返す。jsongetterは自動的にContent-Typeをチェックし、application/jsonでない場合はエラーを投げる。また、レスポンスは一度パースされるとキャッシュされる仕組みになっている。
Learnt from: Miya096jp
PR: fjordllc/bootcamp#8877
File: app/javascript/check-stamp.js:22-24
Timestamp: 2025-07-04T07:15:17.639Z
Learning: Rails request.jsライブラリでは、FetchResponseクラスのjsonプロパティはgetterとして定義されており、response.json()ではなくresponse.jsonとして使用する必要がある。
Learnt from: Miya096jp
PR: fjordllc/bootcamp#8877
File: app/javascript/check-stamp.js:22-24
Timestamp: 2025-07-04T07:15:17.639Z
Learning: Rails request.jsライブラリでは、FetchResponseクラスのjsonプロパティはgetterとして定義されており、response.json()ではなくresponse.jsonとして使用する必要がある。また、Content-Typeがapplication/jsonでない場合は自動的にエラーを投げる安全性チェックも含まれている。
|
@tyrrell-IH |
|
@smallmonkeykey |
|
@tyrrell-IH |
|
@tyrrell-IH そのため、レビュー後の修正時間も含めるとリリースが早くとも10月末〜11月になってしまう見込みのため、今回は別の方にお願いさせていただこうと思っております。 せっかくご快諾いただいたのに本当にすみません。 私自身、見込みの時間などを考慮したうえで、一度しっかり考えてから「お願いします」とお伝えすべきでした。本当にすみません。 また別の機会にぜひお願いできれば嬉しいです。 ご迷惑をおかけして本当にすみません🙇♂️ |
|
@Miya096jp 可能であれば10日以内にご確認いただけますと大変助かります。 |
|
お疲れ様です！ |
|
@Miya096jp |
|
|
||
| def destroy | ||
| current_user.bookmarks.find(params[:id]).destroy | ||
| @bookmarks = current_user.bookmarks.includes(bookmarkable: :user).order(created_at: :desc, id: :desc).page(params[:page]).per(PAGER_NUMBER) |
There was a problem hiding this comment.
-
@bookmarksの取得が重複しているので、set_bookmarksにまとめて、indexだけbefore_action指定、destoryは直接呼び出しなどにするとスッキリして見やすくなると思いました。
-
これは質問ですが、preloadではなくincludesにした理由があれば教えていただけると嬉しいです🙏
There was a problem hiding this comment.
ご指摘ありがとうございます！修正しました🙏
これは質問ですが、preloadではなくincludesにした理由があれば教えていただけると嬉しいです🙏
今回の場合は内部的に preload が使われると思いますが、includes だと必要に応じて preload / eager_load を切り替えてくれるので便利で、こちらを選びました ！
| @@ -0,0 +1,28 @@ | |||
| import { destroy } from '@rails/request.js' | |||
|
|
|||
| document.addEventListener('DOMContentLoaded', () => { | |||
There was a problem hiding this comment.
ブックマークの削除後に、編集ボタンが効かなくなるようです。
編集ボタンのON/OFFや削除ボタンの表示/非表示の制御をapp/javascript/watches.jsで行なっているため、ブックマーク削除後にページがreloadされてapp/javascript/bookmarks-delete-button.jsのDOMContentLoadedが発火した時点で制御が切り替わり、ボタン関連の制御が効かなくなっているみたいです。
👇次の流れで動作確認しました
| 操作 | bookmark | DOMContentLoaded |
|---|---|---|
| 1 | 編集トグルをONにして削除ボタンを表示する | watches.js |
| 2 | 削除ボタンを押してbookmarkを削除 | bookmarks-delete-button.js |
| 3 | 編集はOFFに戻るが、削除ボタンは表示されたまま | bookmarks-delete-button.js |
| 4 | 編集ON/OFF切り替えても削除ボタンの表示/非表示が切り替わらない(常に表示) | bookmarks-delete-button.js |
watches.jsのように、bookmark用に一連のDOM操作を1ファイルにまとめる必要があるんじゃないかと思います。
There was a problem hiding this comment.
ご指摘と動作確認までしてくださってありがとうございます 🙇♀️
編集ボタンによる削除ボタンの表示と非表示は bookmarks-edit-button.js で制御しているのですが、
削除後に bookmarks-delete-button.jsが実行された後の処理に、編集ボタンの制御が抜けていたのが原因でした。
そのため、ファイルをまとめるのではなく、削除後にも編集ボタンの制御が正しく動作するように修正しました。
確認不足の箇所を気づいていただけてとても助かりました 🙇♀️
smallmonkeykey
force-pushed
the
chore/migrate-bookmarks-to-rails-view
branch
from
7dd09ba to
ac57e76
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/views/current_user/bookmarks/_list.html.slim (2)
52-53: ビューロジックのデコレーター化を検討してくださいタイトル表示のロジック（
Talkの場合とそれ以外の分岐）がビュー内に記述されています。コーディングガイドラインに従い、このようなロジックはデコレーターパターンで実装することを検討してください。例：
BookmarkDecoratorにdisplay_titleメソッドを追加# app/decorators/bookmark_decorator.rb def display_title if bookmarkable_type == 'Talk' "#{bookmarkable.user.long_name} さんの相談部屋" else bookmarkable.title end endビューでは：
= link_to bookmark.display_title, polymorphic_path(bookmark.bookmarkable), class: '...'As per coding guidelines
67-68: 日付表示ロジックもデコレーター化を検討してください日付の取得ロジック（
Reportの場合はreported_on.to_time、それ以外はcreated_at）も、先述のタイトル表示と同様にデコレーターに移動することで、ビューの可読性が向上します。例：
BookmarkDecoratorにdisplay_dateメソッドを追加def display_date bookmarkable_type == 'Report' ? bookmarkable.reported_on.to_time : bookmarkable.created_at endAs per coding guidelines
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
app/controllers/api/bookmarks_controller.rb(1 hunks)app/controllers/current_user/bookmarks_controller.rb(1 hunks)app/javascript/bookmarks-delete-button.js(1 hunks)app/javascript/components/Bookmarks.jsx(0 hunks)app/javascript/packs/application.js(1 hunks)app/views/current_user/bookmarks/_list.html.slim(1 hunks)app/views/current_user/bookmarks/index.html.slim(1 hunks)config/routes/current_user.rb(1 hunks)test/system/bookmark/talks_test.rb(1 hunks)test/system/current_user/bookmarks_test.rb(3 hunks)
💤 Files with no reviewable changes (1)
- app/javascript/components/Bookmarks.jsx
🚧 Files skipped from review as they are similar to previous changes (4)
- app/javascript/packs/application.js
- app/views/current_user/bookmarks/index.html.slim
- app/controllers/api/bookmarks_controller.rb
- app/javascript/bookmarks-delete-button.js
🧰 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/current_user/bookmarks_test.rbconfig/routes/current_user.rbapp/controllers/current_user/bookmarks_controller.rbtest/system/bookmark/talks_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/current_user/bookmarks_test.rbtest/system/bookmark/talks_test.rb
🧠 Learnings (5)
📚 Learning: 2025-07-26T01:11:03.921Z
Learnt from: Miya096jp
PR: fjordllc/bootcamp#8877
File: app/javascript/watches.js:78-78
Timestamp: 2025-07-26T01:11:03.921Z
Learning: Rails request.jsのFetchResponseクラスでは、text、json、htmlプロパティはgetterとして定義されており、response.text()ではなくresponse.textとしてアクセスする必要がある。getterは自動的にレスポンスをキャッシュし、jsonの場合はContent-Typeの検証も行う。
Applied to files:
app/views/current_user/bookmarks/_list.html.slim
📚 Learning: 2025-07-04T07:17:55.949Z
Learnt from: Miya096jp
PR: fjordllc/bootcamp#0
File: :0-0
Timestamp: 2025-07-04T07:17:55.949Z
Learning: Rails request.jsのFetchResponseクラスでは、json、text、htmlなどはgetterとして定義されており、response.json()ではなくresponse.jsonとしてアクセスする必要がある。また、getterはContent-Typeの自動チェック機能を含んでいる。
Applied to files:
app/views/current_user/bookmarks/_list.html.slim
📚 Learning: 2025-07-04T07:17:55.949Z
Learnt from: Miya096jp
PR: fjordllc/bootcamp#0
File: :0-0
Timestamp: 2025-07-04T07:17:55.949Z
Learning: Rails request.jsのFetchResponseクラスでは、jsonとtextはgetterとして定義されており、それぞれPromiseを返す。jsongetterは自動的にContent-Typeをチェックし、application/jsonでない場合はエラーを投げる。また、レスポンスは一度パースされるとキャッシュされる仕組みになっている。
Applied to files:
app/views/current_user/bookmarks/_list.html.slim
📚 Learning: 2025-07-04T07:15:17.639Z
Learnt from: Miya096jp
PR: fjordllc/bootcamp#8877
File: app/javascript/check-stamp.js:22-24
Timestamp: 2025-07-04T07:15:17.639Z
Learning: Rails request.jsライブラリでは、FetchResponseクラスのjsonプロパティはgetterとして定義されており、response.json()ではなくresponse.jsonとして使用する必要がある。
Applied to files:
app/views/current_user/bookmarks/_list.html.slim
📚 Learning: 2025-07-04T07:15:17.639Z
Learnt from: Miya096jp
PR: fjordllc/bootcamp#8877
File: app/javascript/check-stamp.js:22-24
Timestamp: 2025-07-04T07:15:17.639Z
Learning: Rails request.jsライブラリでは、FetchResponseクラスのjsonプロパティはgetterとして定義されており、response.json()ではなくresponse.jsonとして使用する必要がある。また、Content-Typeがapplication/jsonでない場合は自動的にエラーを投げる安全性チェックも含まれている。
Applied to files:
app/views/current_user/bookmarks/_list.html.slim
🧬 Code graph analysis (2)
app/controllers/current_user/bookmarks_controller.rb (1)
app/controllers/api/bookmarks_controller.rb (3)
index(3-41)index(6-17)destroy(31-34)
test/system/bookmark/talks_test.rb (1)
app/decorators/user_decorator.rb (1)
long_name(40-42)
⏰ 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 (10)
config/routes/current_user.rb (1)
8-8: ルーティング追加は適切です
destroyアクションの追加により、ブックマークの削除エンドポイントが正しく定義されています。app/controllers/current_user/bookmarks_controller.rb (2)
4-4: ページネーション定数の定義は良好です
PAGER_NUMBERを定数として定義することで、ページネーションの設定が明確になっています。API側のコントローラーとも一貫性があります。
10-14: 削除アクションの実装は適切です
current_user.bookmarks.findによるスコープ化で認可が正しく行われており、削除後にset_bookmarksを呼び出してリストを再読み込みし、パーシャルをレンダリングする流れは問題ありません。test/system/bookmark/talks_test.rb (2)
14-14: 条件付きアサーションのロジックを確認してください
if page.has_text?(@decorated_user.long_name)の条件により、long_nameが表示されていない場合はアサーションがスキップされます。これは意図的な実装でしょうか？フィクスチャーデータのバリエーションに対応するための防御的な実装であれば問題ありませんが、テストが期待通りの検証を行っているか確認してください。
16-18: ページネーションのテストは適切です2ページ目へのリンクの存在確認、ページ遷移、遷移後のコンテンツ確認が正しく行われています。
test/system/current_user/bookmarks_test.rb (2)
29-29: セレクターの更新は正しいです新しいサーバーレンダリングのテンプレートに合わせて、セレクターが
input#bookmark_editに更新されています。
100-102: スコープの変更は適切ですページネーションのアサーションが
.page-main内に正しくスコープされており、新しいサーバーレンダリングの構造と一致しています。app/views/current_user/bookmarks/_list.html.slim (3)
1-14: 空の状態の表示は適切ですブックマークが存在しない場合のメッセージとアイコンがシンプルに実装されています。
35-36: ページネーションの条件分岐は適切です
@bookmarks.total_pages > 1の条件でページネーションを表示し、リストの上下に配置する実装は正しいです。
72-73: 削除ボタンの実装は適切です
data-url属性の設定と JavaScript との連携は正しく実装されています。過去のレビューで確認された通り、Rails request.js の getter を使用した実装も問題ありません。
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
app/javascript/bookmarks-utils.js (1)
4-6: モダンなJavaScript構文の使用を検討してください。C言語スタイルのforループの代わりに
for...ofを使用することで、コードがより簡潔で読みやすくなります。export function toggleDeleteButton(bookMarksEditButton, bookmarkDeleteButton) { bookMarksEditButton.addEventListener('click', () => { + const display = bookMarksEditButton.checked ? 'block' : 'none' + for (const button of bookmarkDeleteButton) { + button.style.display = display + } - if (bookMarksEditButton.checked) { - for (let i = 0; i < bookmarkDeleteButton.length; i++) { - bookmarkDeleteButton[i].style.display = 'block' - } - } else { - for (let i = 0; i < bookmarkDeleteButton.length; i++) { - bookmarkDeleteButton[i].style.display = 'none' - } - } }) }Also applies to: 8-10
app/controllers/current_user/bookmarks_controller.rb (3)
4-4: 定数の重複を解消することを検討してください
PAGER_NUMBER = 20はAPI::BookmarksControllerにも同じ値で定義されています。将来的な保守性向上のため、共通の定数として抽出することを検討してください（例：ApplicationControllerや専用の定数モジュール）。
10-14: 削除処理は正しく動作しますが、さらなる改善の余地がありますセキュリティスコープは適切です（
current_user.bookmarksで絞り込み済み）。ただし、set_bookmarksを手動で呼び出すパターンは、after_actionを使用するか、インライン化することでより洗練できます。例えば、
after_actionを使用する場合：- before_action :set_bookmarks, only: [:index] + before_action :set_bookmarks, only: [:index] + after_action :set_bookmarks, only: [:destroy] def destroy current_user.bookmarks.find(params[:id]).destroy - set_bookmarks render partial: 'current_user/bookmarks/list' end
18-19:includesの使用について確認と提案Miya096jp さんの過去のコメントにもありましたが、
includesを選択された理由を伺えますか？現在のクエリでは
bookmarkableやuserに対するwhere条件がないため、preloadの方が予測可能で効率的です。また、API::BookmarksControllerではpreloadが使用されており、一貫性の観点からもpreloadへの変更を推奨します。def set_bookmarks - @bookmarks = current_user.bookmarks.includes(bookmarkable: :user).order(created_at: :desc, id: :desc).page(params[:page]).per(PAGER_NUMBER) + @bookmarks = current_user.bookmarks.preload(bookmarkable: :user).order(created_at: :desc, id: :desc).page(params[:page]).per(PAGER_NUMBER) end
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/controllers/current_user/bookmarks_controller.rb(1 hunks)app/javascript/bookmarks-delete-button.js(1 hunks)app/javascript/bookmarks-edit-button.js(2 hunks)app/javascript/bookmarks-utils.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/javascript/bookmarks-delete-button.js
🧰 Additional context used
📓 Path-based instructions (2)
**/*.js
⚙️ CodeRabbit configuration file
**/*.js: - どうしても避けられない時以外に新規にVue.js, Reactのコードを追加しない。
- fetchメソッドの代わりにrequest.jsを使う。
Files:
app/javascript/bookmarks-utils.jsapp/javascript/bookmarks-edit-button.js
**/*.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/current_user/bookmarks_controller.rb
🧬 Code graph analysis (3)
app/javascript/bookmarks-utils.js (1)
app/javascript/bookmarks-edit-button.js (3)
bookMarksEditButton(4-4)bookmarkDeleteButton(5-7)i(9-9)
app/javascript/bookmarks-edit-button.js (1)
app/javascript/bookmarks-utils.js (1)
toggleDeleteButton(1-13)
app/controllers/current_user/bookmarks_controller.rb (1)
app/controllers/api/bookmarks_controller.rb (3)
index(3-41)index(6-17)destroy(31-34)
⏰ 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/bookmarks-edit-button.js (2)
1-1: LGTM!
bookmarks-utils.jsからの適切なインポートです。
13-13: LGTM!
toggleDeleteButtonの呼び出しが正しく実装されています。以前のインラインロジックを共通ユーティリティに置き換えることで、コードの保守性が向上しています。app/controllers/current_user/bookmarks_controller.rb (1)
6-8: 実装は適切です
before_actionを使用して@bookmarksを設定し、indexアクションをシンプルに保つパターンは Rails の慣習に従っており、適切です。
app/javascript/bookmarks-utils.js
Outdated
| export function toggleDeleteButton(bookMarksEditButton, bookmarkDeleteButton) { | ||
| bookMarksEditButton.addEventListener('click', () => { | ||
| if (bookMarksEditButton.checked) { | ||
| for (let i = 0; i < bookmarkDeleteButton.length; i++) { | ||
| bookmarkDeleteButton[i].style.display = 'block' | ||
| } | ||
| } else { | ||
| for (let i = 0; i < bookmarkDeleteButton.length; i++) { | ||
| bookmarkDeleteButton[i].style.display = 'none' | ||
| } | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
イベントリスナーのリークの可能性があります。
toggleDeleteButtonが呼び出されるたびに、bookMarksEditButtonに新しいclickイベントリスナーが追加されますが、古いリスナーは削除されません。現在のコードではDOMContentLoaded時に1回だけ呼び出されているため問題ありませんが、将来的に複数回呼び出される可能性がある場合は、リスナーが重複して登録されてしまいます。
次のいずれかの対処を検討してください:
対処法1: リスナーを一度だけ登録する現在の使用パターンを維持する（推奨）
- 現在の実装のままで問題ありません（
bookmarks-edit-button.jsで1回だけ呼び出されているため）
対処法2: 将来の再利用性を考慮して、リスナーの重複登録を防ぐ
export function toggleDeleteButton(bookMarksEditButton, bookmarkDeleteButton) {
+ // 既存のリスナーを削除してから新しいリスナーを追加
+ const handler = () => {
- bookMarksEditButton.addEventListener('click', () => {
if (bookMarksEditButton.checked) {
for (let i = 0; i < bookmarkDeleteButton.length; i++) {
bookmarkDeleteButton[i].style.display = 'block'
}
} else {
for (let i = 0; i < bookmarkDeleteButton.length; i++) {
bookmarkDeleteButton[i].style.display = 'none'
}
}
- })
+ }
+ bookMarksEditButton.removeEventListener('click', handler)
+ bookMarksEditButton.addEventListener('click', handler)
}注: この変更を適用する場合は、handlerへの参照を保持する必要があります。
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In app/javascript/bookmarks-utils.js around lines 1-13 the function adds an
anonymous click listener each time it's called which can lead to duplicated
listeners; convert the listener to a named handler and attach it idempotently:
define the handler function (e.g. toggleDeleteHandler) that toggles display for
bookmarkDeleteButton, store the handler reference (or attach it once) and when
registering call removeEventListener with that same reference before
addEventListener (or guard by checking a data-attribute/flag on the button to
only attach once) so repeated calls won't duplicate listeners.
There was a problem hiding this comment.
イベントリスナーのリークの可能性があります。
使っていないのにメモリを保持し続けるメモリリークの指摘であっていますか？
Chrome DevTools のMemoryで detached event listener を確認しましたが、Collect garbageを押して、GC によって解放されることを確認したので、現状では問題ない認識です。
There was a problem hiding this comment.
ご確認ありがとうございます。私の説明が不正確でした。
「メモリリーク」ではなく、イベントリスナーの重複登録を懸念していました。具体的には、toggleDeleteButtonが複数回呼び出された場合、同じボタンに複数のclickリスナーが追加され、1回のクリックで複数回処理が実行される可能性を指摘していました。
ただし、現在の実装ではbookmarks-edit-button.jsでDOMContentLoaded時に1回だけ呼び出されているため、実際には問題は発生しません。
Chrome DevToolsでGCによる解放を確認されたとのことで、メモリ管理の観点でも問題ないことが確認できていますね。現状の実装で問題ありません。
将来的にこの関数を複数回呼び出す可能性がある場合のみ、リスナーの重複登録を防ぐ実装を検討していただければと思います。
There was a problem hiding this comment.
ご丁寧に補足ありがとうございます！🙇♀️
なるほど、懸念されていたのはメモリリークではなく イベントリスナーの重複登録 だったのですね。
現状の実装で問題ないことも確認できて安心しました。
There was a problem hiding this comment.
こちらこそ、ご確認いただきありがとうございました!
ご理解いただけて良かったです。現在の実装で問題なく動作していることが確認できましたので、このままで大丈夫です。今後もお気軽にご質問ください! 😊
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/javascript/bookmarks-edit-button.js (1)
8-14: 存在チェックと初期化ロジックの改善を検討してください以下の点について改善の余地があります：
HTMLCollectionの存在チェック: Line 8の
bookmarkDeleteButtonの存在チェックは不十分です。getElementsByClassNameは常にHTMLCollectionを返すため、空のコレクションでもtruthyと評価されます。ループの一貫性: Lines 9-11では古いスタイルの
forループを使用していますが、toggleDeleteButton関数内ではfor...ofを使用しています。以下の修正を適用して、存在チェックとコードの一貫性を改善してください：
- if (bookMarksEditButton && bookmarkDeleteButton) { + if (bookMarksEditButton && bookmarkDeleteButton.length > 0) { - for (let i = 0; i < bookmarkDeleteButton.length; i++) { - bookmarkDeleteButton[i].style.display = 'none' + for (const button of bookmarkDeleteButton) { + button.style.display = 'none' } toggleDeleteButton(bookMarksEditButton, bookmarkDeleteButton) }または、CSSで初期状態を
display: noneに設定することで、JavaScriptでの初期化処理を削減できます。
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/javascript/bookmarks-edit-button.js(2 hunks)app/javascript/bookmarks-utils.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/javascript/bookmarks-utils.js
🧰 Additional context used
📓 Path-based instructions (1)
**/*.js
⚙️ CodeRabbit configuration file
**/*.js: - どうしても避けられない時以外に新規にVue.js, Reactのコードを追加しない。
- fetchメソッドの代わりにrequest.jsを使う。
Files:
app/javascript/bookmarks-edit-button.js
🧬 Code graph analysis (1)
app/javascript/bookmarks-edit-button.js (1)
app/javascript/bookmarks-utils.js (1)
toggleDeleteButton(1-8)
⏰ 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/javascript/bookmarks-edit-button.js (1)
1-1: LGTM: インポート文は適切です
toggleDeleteButton関数をbookmarks-utilsから適切にインポートしています。共通のユーティリティ関数として切り出すことで、コードの再利用性と保守性が向上しています。
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/javascript/bookmarks-edit-button.js (1)
9-14: 実装は正常に動作しますが、軽微な改善の余地があります現在の実装では、初期状態の非表示処理(10-12行目)と
toggleDeleteButton内のロジックで一部重複があります。ただし、これは初期状態を確実にするための防御的プログラミングとして妥当です。将来的には、
toggleDeleteButtonユーティリティ関数に初期状態の設定も含めることで、呼び出し側のコードをさらに簡潔にできる可能性があります。例えば、
bookmarks-utils.jsを以下のように拡張できます:export function toggleDeleteButton(bookMarksEditButton, bookmarkDeleteButton, initializeHidden = true) { // 初期状態を設定 if (initializeHidden) { for (const button of bookmarkDeleteButton) { button.style.display = 'none' } } bookMarksEditButton.addEventListener('click', () => { const displayStyle = bookMarksEditButton.checked ? 'block' : 'none' for (const button of bookmarkDeleteButton) { button.style.display = displayStyle } }) }そして、このファイルでは以下のように簡潔に呼び出せます:
- if (bookMarksEditButton && bookmarkDeleteButton.length > 0) { - for (const button of bookmarkDeleteButton) { - button.style.display = 'none' - } - - toggleDeleteButton(bookMarksEditButton, bookmarkDeleteButton) - } + if (bookMarksEditButton && bookmarkDeleteButton.length > 0) { + toggleDeleteButton(bookMarksEditButton, bookmarkDeleteButton) + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/javascript/bookmarks-edit-button.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.js
⚙️ CodeRabbit configuration file
**/*.js: - どうしても避けられない時以外に新規にVue.js, Reactのコードを追加しない。
- fetchメソッドの代わりにrequest.jsを使う。
Files:
app/javascript/bookmarks-edit-button.js
🧬 Code graph analysis (1)
app/javascript/bookmarks-edit-button.js (1)
app/javascript/bookmarks-utils.js (1)
toggleDeleteButton(1-8)
⏰ 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/javascript/bookmarks-edit-button.js (1)
1-1: リファクタリングは適切です共通ユーティリティ関数への抽出により、コードの再利用性が向上しています。
smallmonkeykey
force-pushed
the
chore/migrate-bookmarks-to-rails-view
branch
from
065d27c to
eef5957
Compare
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/current_user/bookmarks/index.html.slim (1)
1-2: タイトルとメタタグが削除されていますページタイトル（
title）とメタディスクリプション（set_meta_tags）が削除されています。これによりSEO対策やブラウザのタブ表示に影響が出ます。元のコードにあった以下の行を残すべきではないでしょうか。- title 'ブックマーク一覧' - set_meta_tags description: 'ブックマーク一覧ページです。'
🧹 Nitpick comments (3)
app/javascript/bookmarks-delete-button.js (3)
4-7: イベント委譲の実装は適切だが、DOMContentLoadedは不要の可能性イベント委譲を使用しているため、
DOMContentLoadedでラップする必要はありません。削除後にコンテンツが動的に置き換えられても、documentへのイベントリスナーは継続して機能します。次のように簡略化できます:
-document.addEventListener('DOMContentLoaded', () => { - document.addEventListener('click', async (event) => { +document.addEventListener('click', async (event) => { + const deleteButton = event.target.closest('.bookmark-delete-button') + if (!deleteButton) return + + event.preventDefault() + + const url = deleteButton.dataset.url + + try { // ... rest of the code - }) -}) + } catch (error) { + console.warn(error) + } +})
23-24: outerHTML置き換えの潜在的な問題
.page-mainが存在しない場合のエラー処理がありません。また、outerHTMLは要素自体を置き換えるため、サーバーから返されるHTMLに.page-main要素が含まれていることを前提としています。より堅牢にするため、存在チェックと適切なエラーハンドリングを追加することを推奨します:
- const html = await response.text - document.querySelector('.page-main').outerHTML = html + const html = await response.text + const pageMain = document.querySelector('.page-main') + if (!pageMain) { + throw new Error('ページ要素が見つかりません') + } + pageMain.outerHTML = html
38-40: ユーザーへのエラーフィードバックが不足削除が失敗した場合、
console.warnでログ出力するのみで、ユーザーには何も通知されません。ユーザーエクスペリエンス向上のため、エラーメッセージを表示することを推奨します。例えば、次のようにアラートまたはトースト通知を追加できます:
} catch (error) { console.warn(error) + alert('ブックマークの削除に失敗しました。') }より洗練された方法として、既存の通知システム(Flashメッセージなど)を使用することも検討してください。
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
app/controllers/api/bookmarks_controller.rb(1 hunks)app/controllers/current_user/bookmarks_controller.rb(1 hunks)app/javascript/bookmarks-delete-button.js(1 hunks)app/javascript/bookmarks-edit-button.js(1 hunks)app/javascript/bookmarks-utils.js(1 hunks)app/javascript/components/Bookmarks.jsx(0 hunks)app/javascript/packs/application.js(1 hunks)app/views/current_user/bookmarks/_list.html.slim(1 hunks)app/views/current_user/bookmarks/index.html.slim(1 hunks)config/routes/current_user.rb(1 hunks)test/system/bookmark/talks_test.rb(1 hunks)test/system/current_user/bookmarks_test.rb(3 hunks)
💤 Files with no reviewable changes (1)
- app/javascript/components/Bookmarks.jsx
🚧 Files skipped from review as they are similar to previous changes (6)
- app/javascript/bookmarks-edit-button.js
- app/javascript/bookmarks-utils.js
- app/javascript/packs/application.js
- test/system/bookmark/talks_test.rb
- config/routes/current_user.rb
- app/views/current_user/bookmarks/_list.html.slim
🧰 Additional context used
📓 Path-based instructions (3)
**/*.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/current_user/bookmarks_test.rbapp/controllers/current_user/bookmarks_controller.rbapp/controllers/api/bookmarks_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/current_user/bookmarks_test.rb
**/*.js
⚙️ CodeRabbit configuration file
**/*.js: - どうしても避けられない時以外に新規にVue.js, Reactのコードを追加しない。
- fetchメソッドの代わりにrequest.jsを使う。
Files:
app/javascript/bookmarks-delete-button.js
🧬 Code graph analysis (2)
app/controllers/current_user/bookmarks_controller.rb (1)
app/controllers/api/bookmarks_controller.rb (3)
index(3-41)index(6-17)destroy(31-34)
app/javascript/bookmarks-delete-button.js (2)
app/controllers/current_user/bookmarks_controller.rb (1)
destroy(10-14)app/javascript/bookmarks-utils.js (1)
toggleDeleteButton(1-8)
⏰ 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 (9)
app/views/current_user/bookmarks/index.html.slim (1)
7-7: パーシャルへの委譲は適切ですサーバーサイドレンダリングへの移行に伴い、パーシャル
current_user/bookmarks/listにレンダリングを委譲する実装は妥当です。app/controllers/api/bookmarks_controller.rb (1)
4-4: ページネーション定数の統一は適切です
PAGER_NUMBERを25から20に変更し、CurrentUser::BookmarksControllerと統一したことで、コードベース全体の一貫性が向上しています。test/system/current_user/bookmarks_test.rb (2)
29-29: セレクタ更新は適切ですReactコンポーネントから Rails ビューへの移行に伴い、セレクタを
input#bookmark_editに更新したのは正しい対応です。
100-102: ページネーションテストのスコープ変更は妥当ですページネーションの検証スコープを
.page-mainに変更したことで、サーバーサイドレンダリングされた新しいDOM構造に対応できています。app/controllers/current_user/bookmarks_controller.rb (3)
4-8: コントローラー構造は適切です
PAGER_NUMBER定数の導入、before_actionによるset_bookmarksの呼び出し、空のindexアクションという構成は、Railsの標準的なパターンに従っており、Fat Controllerを避ける良い実装です。
10-14: destroy アクションの実装は適切ですユーザースコープを適用した削除処理（過去のレビューで修正済み）、ブックマークの再読み込み、パーシャルのレンダリングという流れは、AJAXによる非同期削除の実装として妥当です。
18-20:preloadとincludesの使い分けを確認してください過去のコメントで「
includesだと必要に応じて preload / eager_load を切り替えてくれる」と説明されていましたが、実際のコードではpreloadが使用されています。ビューで
bookmarkableとbookmarkable.userの両方を参照する場合、includesの方が適切ではないでしょうか？includesは必要に応じて JOIN を使うeager_loadに切り替わるため、より柔軟です。
preloadを意図的に選択した理由があればそのままで問題ありませんが、もし過去の議論通りincludesにする予定であれば、以下のように修正してください：- @bookmarks = current_user.bookmarks.preload(bookmarkable: :user).order(created_at: :desc, id: :desc).page(params[:page]).per(PAGER_NUMBER) + @bookmarks = current_user.bookmarks.includes(bookmarkable: :user).order(created_at: :desc, id: :desc).page(params[:page]).per(PAGER_NUMBER)app/javascript/bookmarks-delete-button.js (2)
1-2: 適切なライブラリの使用コーディングガイドラインに従い、
@rails/request.jsを使用しています。適切です。
9-17: AJAX削除リクエストの実装は適切
@rails/request.jsのdestroyメソッドを正しく使用し、サーバーが返すHTML部分テンプレートに合わせてAccept: 'text/html'を指定しています。
| const bookMarksEditButton = document.getElementById('bookmark_edit') | ||
| const bookmarkDeleteButton = document.getElementsByClassName( | ||
| 'js-bookmark-delete-button' | ||
| ) | ||
| if (bookMarksEditButton && bookmarkDeleteButton) { | ||
| bookMarksEditButton.checked = true | ||
|
|
||
| for (let i = 0; i < bookmarkDeleteButton.length; i++) { | ||
| bookmarkDeleteButton[i].style.display = 'block' | ||
| } | ||
| toggleDeleteButton(bookMarksEditButton, bookmarkDeleteButton) | ||
| } |
There was a problem hiding this comment.
削除後の状態管理に問題あり
削除後に編集モードを強制的にONにし、削除ボタンを表示していますが、これはユーザーの意図と異なる可能性があります。また、手動でボタンを表示した後にtoggleDeleteButtonを呼び出すのは冗長です。
主な問題点:
bookMarksEditButton.checked = trueにより、削除のたびに編集モードが強制的にONになる- ループで手動表示する処理(33-35行目)が
toggleDeleteButtonの責務と重複 - DOM置き換え後の状態が不整合になる可能性
次のように、現在の編集モード状態を保持し、冗長な処理を削除することを推奨します:
const bookMarksEditButton = document.getElementById('bookmark_edit')
const bookmarkDeleteButton = document.getElementsByClassName(
'js-bookmark-delete-button'
)
if (bookMarksEditButton && bookmarkDeleteButton) {
- bookMarksEditButton.checked = true
-
- for (let i = 0; i < bookmarkDeleteButton.length; i++) {
- bookmarkDeleteButton[i].style.display = 'block'
- }
toggleDeleteButton(bookMarksEditButton, bookmarkDeleteButton)
+ // 現在のチェック状態に基づいて表示を更新
+ const displayStyle = bookMarksEditButton.checked ? 'block' : 'none'
+ for (const button of bookmarkDeleteButton) {
+ button.style.display = displayStyle
+ }
}ただし、より良い解決策はbookmarks-utils.jsのtoggleDeleteButton関数を拡張し、初期状態の設定も行えるようにすることです。
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const bookMarksEditButton = document.getElementById('bookmark_edit') | |
| const bookmarkDeleteButton = document.getElementsByClassName( | |
| 'js-bookmark-delete-button' | |
| ) | |
| if (bookMarksEditButton && bookmarkDeleteButton) { | |
| bookMarksEditButton.checked = true | |
| for (let i = 0; i < bookmarkDeleteButton.length; i++) { | |
| bookmarkDeleteButton[i].style.display = 'block' | |
| } | |
| toggleDeleteButton(bookMarksEditButton, bookmarkDeleteButton) | |
| } | |
| const bookMarksEditButton = document.getElementById('bookmark_edit') | |
| const bookmarkDeleteButton = document.getElementsByClassName( | |
| 'js-bookmark-delete-button' | |
| ) | |
| if (bookMarksEditButton && bookmarkDeleteButton) { | |
| // 再度イベントリスナーを（再）設定 | |
| toggleDeleteButton(bookMarksEditButton, bookmarkDeleteButton) | |
| // 初期表示を現在のチェック状態に合わせて更新 | |
| const displayStyle = bookMarksEditButton.checked ? 'block' : 'none' | |
| for (const button of bookmarkDeleteButton) { | |
| button.style.display = displayStyle | |
| } | |
| } |
🤖 Prompt for AI Agents
In app/javascript/bookmarks-delete-button.js around lines 26 to 37, the current
code force-sets edit mode (bookMarksEditButton.checked = true) and then manually
shows each delete button in a loop before calling toggleDeleteButton, which
forces edit-mode on every delete and duplicates responsibilities; remove the
forced assignment and the manual loop, and instead preserve the current checked
state of bookMarksEditButton and call toggleDeleteButton(bookMarksEditButton,
bookmarkDeleteButton) to let that function control visibility; alternatively,
extend bookmarks-utils.js toggleDeleteButton to accept an optional init flag so
it can also apply the correct initial visibility after DOM replacement and call
it here with that flag when needed.
There was a problem hiding this comment.
現状のReactと同様の動きは、削除をした後も、編集ボタンをクリックしていない限り削除ボタンは開いたままです。
そのため bookMarksEditButton.checked = trueをしています。
なので、
bookMarksEditButton.checked = trueは残し、
- for (let i = 0; i < bookmarkDeleteButton.length; i++) {
- bookmarkDeleteButton[i].style.display = 'block'
- }
は削除するというのはどうでしょうか？
There was a problem hiding this comment.
@smallmonkeykey さん、ご説明ありがとうございます！
Reactの既存動作を維持する意図を理解しました。削除後も編集モードを保持するために bookMarksEditButton.checked = true が必要なのですね。
ご提案の通り、以下のようにするのが最適です:
bookMarksEditButton.checked = trueは残す（編集モード維持のため）- 手動でボタンを表示するループ（33-35行目）は削除（
toggleDeleteButtonが checked 状態に基づいて表示を制御するため冗長）
この変更により、コードがシンプルになり、toggleDeleteButton の責務も明確になります。👍
if (bookMarksEditButton && bookmarkDeleteButton) {
bookMarksEditButton.checked = true
toggleDeleteButton(bookMarksEditButton, bookmarkDeleteButton)
}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/javascript/bookmarks-delete-button.js (1)
32-33: ユーザーへのエラーフィードバックの追加を推奨します。現在、エラーは
console.warnでのみ出力されており、ユーザーには削除が失敗したことが伝わりません。ユーザー体験向上のため、アラートやトースト通知でエラーを表示することを検討してください。例:
} catch (error) { console.warn(error) + alert(error.message || '削除に失敗しました。') }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/javascript/bookmarks-delete-button.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.js
⚙️ CodeRabbit configuration file
**/*.js: - どうしても避けられない時以外に新規にVue.js, Reactのコードを追加しない。
- fetchメソッドの代わりにrequest.jsを使う。
Files:
app/javascript/bookmarks-delete-button.js
🧬 Code graph analysis (1)
app/javascript/bookmarks-delete-button.js (4)
app/controllers/api/bookmarks_controller.rb (1)
destroy(31-34)app/controllers/current_user/bookmarks_controller.rb (1)
destroy(10-14)app/javascript/bookmarks-edit-button.js (2)
bookMarksEditButton(4-4)bookmarkDeleteButton(5-7)app/javascript/bookmarks-utils.js (1)
toggleDeleteButton(1-8)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build_and_test
🔇 Additional comments (4)
app/javascript/bookmarks-delete-button.js (4)
1-2: LGTM! インポート文は適切です。コーディングガイドラインに従い、
@rails/request.jsを使用しています。
4-11: LGTM! イベント委譲パターンが適切に実装されています。
closest()を使用したイベント委譲により、動的に追加される削除ボタンにも対応できています。
13-17: LGTM! リクエスト設定が適切です。サーバーがHTMLパーシャルを返すため、
responseKind: 'html'とAccept: 'text/html'の指定は正しいです。
19-21: LGTM! レスポンスエラーのハンドリングが適切です。HTTPエラー時に日本語のエラーメッセージとステータスコードを含めており、デバッグしやすくなっています。
| const html = await response.text | ||
| document.querySelector('.page-main').outerHTML = html |
There was a problem hiding this comment.
response.text()の呼び出しが必要です。
response.textではなくresponse.text()とする必要があります。textはメソッドであり、プロパティではありません。
次の修正を適用してください:
- const html = await response.text
+ const html = await response.text()
document.querySelector('.page-main').outerHTML = html📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const html = await response.text | |
| document.querySelector('.page-main').outerHTML = html | |
| const html = await response.text() | |
| document.querySelector('.page-main').outerHTML = html |
🤖 Prompt for AI Agents
In app/javascript/bookmarks-delete-button.js around lines 23 to 24, the code
uses response.text (a method) without invoking it; change it to await
response.text() so the response body is actually read as text before assigning
to html and updating the DOM.
There was a problem hiding this comment.
rails/request.jsを使っているためtextです。
以前もこちらでやりとりをしていますので確認お願いします🙏
#9181 (comment)
There was a problem hiding this comment.
@smallmonkeykey 大変申し訳ございません。ご指摘の通りです。
Rails request.jsのFetchResponseクラスでは、text、json、htmlはgetterとして定義されており、response.text()ではなくresponse.textとしてアクセスする必要があります。現在のコードは正しい実装です。
以前のやり取りを見落としており、誤った指摘をしてしまいました。混乱を招いてしまい申し訳ございません。🙏
🧠 Learnings used
Learnt from: Miya096jp
PR: fjordllc/bootcamp#8877
File: app/javascript/watches.js:78-78
Timestamp: 2025-07-26T01:11:03.921Z
Learning: Rails request.jsのFetchResponseクラスでは、text、json、htmlプロパティはgetterとして定義されており、response.text()ではなくresponse.textとしてアクセスする必要がある。getterは自動的にレスポンスをキャッシュし、jsonの場合はContent-Typeの検証も行う。
Learnt from: Miya096jp
PR: fjordllc/bootcamp#0
File: :0-0
Timestamp: 2025-07-04T07:17:55.949Z
Learning: Rails request.jsのFetchResponseクラスでは、json、text、htmlなどはgetterとして定義されており、response.json()ではなくresponse.jsonとしてアクセスする必要がある。また、getterはContent-Typeの自動チェック機能を含んでいる。
Learnt from: Miya096jp
PR: fjordllc/bootcamp#0
File: :0-0
Timestamp: 2025-07-04T07:17:55.949Z
Learning: Rails request.jsのFetchResponseクラスでは、jsonとtextはgetterとして定義されており、それぞれPromiseを返す。jsongetterは自動的にContent-Typeをチェックし、application/jsonでない場合はエラーを投げる。また、レスポンスは一度パースされるとキャッシュされる仕組みになっている。
Learnt from: Miya096jp
PR: fjordllc/bootcamp#8877
File: app/javascript/check-stamp.js:22-24
Timestamp: 2025-07-04T07:15:17.639Z
Learning: Rails request.jsライブラリでは、FetchResponseクラスのjsonプロパティはgetterとして定義されており、response.json()ではなくresponse.jsonとして使用する必要がある。
| const bookMarksEditButton = document.getElementById('bookmark_edit') | ||
| const bookmarkDeleteButton = document.getElementsByClassName( | ||
| 'js-bookmark-delete-button' | ||
| ) | ||
| bookMarksEditButton.checked = true | ||
| toggleDeleteButton(bookMarksEditButton, bookmarkDeleteButton) |
There was a problem hiding this comment.
イベントリスナーの重複登録とnullチェックの問題があります。
以下の問題があります:
-
イベントリスナーが重複登録される:
toggleDeleteButtonは呼び出されるたびに新しいイベントリスナーを追加します。削除のたびにtoggleDeleteButtonを呼び出すと、リスナーが累積し、編集ボタンをクリックするたびに複数のハンドラーが実行されます。 -
nullチェックが不足:
bookMarksEditButtonがnullの場合、30行目でTypeErrorが発生します。
次の修正を適用してください:
const bookMarksEditButton = document.getElementById('bookmark_edit')
const bookmarkDeleteButton = document.getElementsByClassName(
'js-bookmark-delete-button'
)
- bookMarksEditButton.checked = true
- toggleDeleteButton(bookMarksEditButton, bookmarkDeleteButton)
+ if (bookMarksEditButton && bookmarkDeleteButton.length > 0) {
+ bookMarksEditButton.checked = true
+ // 削除ボタンの表示状態を直接更新(リスナー重複を避ける)
+ for (const button of bookmarkDeleteButton) {
+ button.style.display = 'block'
+ }
+ }代替案: bookmarks-utils.jsのtoggleDeleteButtonを修正して、リスナーを一度だけ登録するか、既存のリスナーを削除してから新しいリスナーを追加するようにすることもできます。ただし、ここでは直接スタイルを更新する方がシンプルです。
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In app/javascript/bookmarks-delete-button.js around lines 26 to 31,
bookMarksEditButton is not null-checked and toggleDeleteButton is called each
time which adds duplicate event listeners; instead, first guard against null
(return early if bookMarksEditButton is falsy), then do not call
toggleDeleteButton repeatedly — either remove this call here and directly update
the delete buttons' visibility/state based on bookMarksEditButton.checked
(iterate over document.getElementsByClassName('js-bookmark-delete-button') and
set style/class accordingly), or ensure toggleDeleteButton in bookmarks-utils.js
only registers its event listener once (remove existing listener before adding
or use a flag). Make the minimal change: add the null check and replace the
toggleDeleteButton call with direct DOM updates so no duplicate listeners are
created.
There was a problem hiding this comment.
イベントリスナーに関しては過去のやり取りで解決していますのでご確認お願いします🙏
#9181 (comment)
nullチェックに関しては、以下で修正しました！
00b6a67
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the commit 00b6a67 to see what null check was added
git show 00b6a67a913aad0b3a1032f70fe118358d435cfa --stat
echo "---"
git show 00b6a67a913aad0b3a1032f70fe118358d435cfa -- app/javascript/bookmarks-delete-button.js
Length of output: 197124
@smallmonkeykey ありがとうございます!
nullチェックの修正を確認しました。bookMarksEditButton と bookmarkDeleteButton.length の両方をチェックしているので、適切な対応です👍
イベントリスナーに関する過去のやり取りについては、私が直接そのディスカッションのURLにアクセスできないため内容を確認できませんが、そちらで既に解決済みとのことですので了解しました。
両方の懸念事項について対応いただきありがとうございました!
smallmonkeykey
force-pushed
the
chore/migrate-bookmarks-to-rails-view
branch
from
00b6a67 to
6ea84ae
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/javascript/bookmarks-delete-button.js (1)
34-36: ユーザーへのエラーフィードバックを検討してください。現在、エラーは
console.warnでログに記録されるだけで、ユーザーには表示されません。削除が失敗した場合、ユーザーが状況を理解できるよう、アラートやトースト通知などでエラーメッセージを表示することを検討してください。例:
} catch (error) { console.warn(error) + alert('ブックマークの削除に失敗しました。ページを再読み込みして再度お試しください。') }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
app/controllers/api/bookmarks_controller.rb(1 hunks)app/controllers/current_user/bookmarks_controller.rb(1 hunks)app/javascript/bookmarks-delete-button.js(1 hunks)app/javascript/bookmarks-edit-button.js(1 hunks)app/javascript/bookmarks-utils.js(1 hunks)app/javascript/components/Bookmarks.jsx(0 hunks)app/javascript/packs/application.js(1 hunks)app/views/current_user/bookmarks/_list.html.slim(1 hunks)app/views/current_user/bookmarks/index.html.slim(1 hunks)config/routes/current_user.rb(1 hunks)test/system/bookmark/talks_test.rb(1 hunks)test/system/current_user/bookmarks_test.rb(3 hunks)
💤 Files with no reviewable changes (1)
- app/javascript/components/Bookmarks.jsx
🚧 Files skipped from review as they are similar to previous changes (5)
- app/javascript/packs/application.js
- config/routes/current_user.rb
- app/controllers/api/bookmarks_controller.rb
- app/views/current_user/bookmarks/_list.html.slim
- app/controllers/current_user/bookmarks_controller.rb
🧰 Additional context used
📓 Path-based instructions (3)
**/*.js
⚙️ CodeRabbit configuration file
**/*.js: - どうしても避けられない時以外に新規にVue.js, Reactのコードを追加しない。
- fetchメソッドの代わりにrequest.jsを使う。
Files:
app/javascript/bookmarks-utils.jsapp/javascript/bookmarks-edit-button.jsapp/javascript/bookmarks-delete-button.js
**/*.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/current_user/bookmarks_test.rbtest/system/bookmark/talks_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/current_user/bookmarks_test.rbtest/system/bookmark/talks_test.rb
🧠 Learnings (5)
📚 Learning: 2025-07-26T01:11:03.921Z
Learnt from: Miya096jp
PR: fjordllc/bootcamp#8877
File: app/javascript/watches.js:78-78
Timestamp: 2025-07-26T01:11:03.921Z
Learning: Rails request.jsのFetchResponseクラスでは、text、json、htmlプロパティはgetterとして定義されており、response.text()ではなくresponse.textとしてアクセスする必要がある。getterは自動的にレスポンスをキャッシュし、jsonの場合はContent-Typeの検証も行う。
Applied to files:
app/javascript/bookmarks-delete-button.js
📚 Learning: 2025-07-04T07:17:55.949Z
Learnt from: Miya096jp
PR: fjordllc/bootcamp#0
File: :0-0
Timestamp: 2025-07-04T07:17:55.949Z
Learning: Rails request.jsのFetchResponseクラスでは、json、text、htmlなどはgetterとして定義されており、response.json()ではなくresponse.jsonとしてアクセスする必要がある。また、getterはContent-Typeの自動チェック機能を含んでいる。
Applied to files:
app/javascript/bookmarks-delete-button.js
📚 Learning: 2025-07-04T07:17:55.949Z
Learnt from: Miya096jp
PR: fjordllc/bootcamp#0
File: :0-0
Timestamp: 2025-07-04T07:17:55.949Z
Learning: Rails request.jsのFetchResponseクラスでは、jsonとtextはgetterとして定義されており、それぞれPromiseを返す。jsongetterは自動的にContent-Typeをチェックし、application/jsonでない場合はエラーを投げる。また、レスポンスは一度パースされるとキャッシュされる仕組みになっている。
Applied to files:
app/javascript/bookmarks-delete-button.js
📚 Learning: 2025-07-04T07:15:17.639Z
Learnt from: Miya096jp
PR: fjordllc/bootcamp#8877
File: app/javascript/check-stamp.js:22-24
Timestamp: 2025-07-04T07:15:17.639Z
Learning: Rails request.jsライブラリでは、FetchResponseクラスのjsonプロパティはgetterとして定義されており、response.json()ではなくresponse.jsonとして使用する必要がある。
Applied to files:
app/javascript/bookmarks-delete-button.js
📚 Learning: 2025-07-04T07:15:17.639Z
Learnt from: Miya096jp
PR: fjordllc/bootcamp#8877
File: app/javascript/check-stamp.js:22-24
Timestamp: 2025-07-04T07:15:17.639Z
Learning: Rails request.jsライブラリでは、FetchResponseクラスのjsonプロパティはgetterとして定義されており、response.json()ではなくresponse.jsonとして使用する必要がある。また、Content-Typeがapplication/jsonでない場合は自動的にエラーを投げる安全性チェックも含まれている。
Applied to files:
app/javascript/bookmarks-delete-button.js
🧬 Code graph analysis (4)
app/javascript/bookmarks-utils.js (1)
app/javascript/bookmarks-edit-button.js (2)
bookMarksEditButton(4-4)bookmarkDeleteButton(5-7)
app/javascript/bookmarks-edit-button.js (1)
app/javascript/bookmarks-utils.js (1)
toggleDeleteButton(1-8)
app/javascript/bookmarks-delete-button.js (3)
app/controllers/current_user/bookmarks_controller.rb (1)
destroy(10-14)app/javascript/bookmarks-edit-button.js (2)
bookMarksEditButton(4-4)bookmarkDeleteButton(5-7)app/javascript/bookmarks-utils.js (1)
toggleDeleteButton(1-8)
test/system/bookmark/talks_test.rb (1)
app/decorators/user_decorator.rb (1)
long_name(40-42)
⏰ 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 (10)
test/system/bookmark/talks_test.rb (1)
16-17: ページネーションのテストが適切に追加されています。サーバー側でレンダリングされるページネーションの動作を検証しており、実装と整合しています。
test/system/current_user/bookmarks_test.rb (1)
29-29: セレクタの更新が適切です。サーバー側レンダリングに合わせて、セレクタが
#bookmark_editおよび.page-mainコンテナに更新されています。テストロジックは変更されておらず、新しいDOM構造と整合しています。Also applies to: 100-102, 122-124
app/javascript/bookmarks-edit-button.js (1)
1-16: 共通ユーティリティへのリファクタリングが適切です。
toggleDeleteButtonユーティリティを使用することで、削除ボタンの表示/非表示ロジックが一元化され、コードの保守性が向上しています。条件チェックの改善（length > 0）とモダンなfor...of構文の使用も適切です。app/javascript/bookmarks-utils.js (1)
1-8: 共通ユーティリティ関数が適切に実装されています。削除ボタンの表示/非表示を制御する関数が適切に実装されており、現在の使用パターン（
DOMContentLoaded時に1回のみ呼び出し）では問題ありません。過去のレビューコメントで懸念されていたイベントリスナーの重複登録については、現在の実装では発生しないことが確認されています。Based on learnings
app/javascript/bookmarks-delete-button.js (5)
1-2: インポートが適切です。
@rails/request.jsの使用はコーディングガイドラインに従っており、共通ユーティリティのインポートも適切です。As per coding guidelines
4-7: イベント委譲パターンが適切に実装されています。
closest()を使用したイベント委譲により、動的に追加される削除ボタンにも対応できます。効率的な実装です。
14-17:destroy()の呼び出しが正しく実装されています。
responseKind: 'html'とAccept: 'text/html'ヘッダーの設定により、サーバーから返されるHTMLパーシャルと整合しています。
19-24: レスポンス処理とDOM置換が適切です。
response.textはrails/request.jsのgetterとして正しく使用されており、エラーハンドリングも適切です。DOM置換により削除後のリストが正しく表示されます。Based on learnings
26-33: 削除後のUI再初期化が適切に実装されています。DOM置換後に編集ボタンと削除ボタンの制御を再設定しています。
bookMarksEditButton.checked = trueにより、Reactの既存動作と同様に削除後も編集モードが維持されます。
toggleDeleteButtonの呼び出しは、DOM置換により古いイベントリスナーが破棄されるため、新しいボタンに対してリスナーを再アタッチするために必要です。これはリスナーの重複ではなく、適切な再初期化です。app/views/current_user/bookmarks/index.html.slim (1)
1-7: レビューコメント無効:titleとset_meta_tagsはまだ存在
index.html.slim の先頭に該当行が残っているため、本コメントは不要です。Likely an incorrect or invalid review comment.
|
@Miya096jp |
- initializerの作成 - 複数イベントを1つのイベントリスナーに統合
- editButtonのイベント登録をDOMContentLoaded内に移動 - handleEditToggleChangeを削除し、無名関数にした - ページ再描画後にも編集モードの状態を維持するようにした
smallmonkeykey
force-pushed
the
chore/migrate-bookmarks-to-rails-view
branch
from
02ba61c to
c48ac75
Compare
|
@okuramasafumi |
|
@okuramasafumi @komagata |
|
@smallmonkeykey マージさせていただきました〜！ |
5 participants
Issue
概要
ブックマーク一覧ページ (Bookmarks.jsx) を脱React化の一環として Rails で書き直しました。
React への依存をなくし、Rails のビューでレンダリングする形に変更しています。
また、ブックマーク削除については非同期処理で実装しています。
変更確認方法
komagataでログインを行うhttp://localhost:3000/current_user/bookmarksにアクセスするScreenshot
スクリーンショットの撮影はありません。
ただ今回
order(created_at: :desc, id: :desc)を追加したことで、fixture データでは順番が変わったように見える場合があります。これは、fixture 内で同じ created_at を持つレコードが複数あるため、id で順序が決まるようになったからです。一方で、実際に新規作成されたデータでは React 実装時と同じ順番で表示されています。挙動は変わっていません。
Summary by CodeRabbit
新機能
変更
テスト
✏️ Tip: You can customize this high-level summary in your review settings.