Conversation
|
@riq0h |
riq0h
left a comment
riq0h
left a comment
There was a problem hiding this comment.
確認しました！ OKと思われるのでApproveさせていただきました🙇
reckyy
force-pushed
the
feature/add-list-of-movies-by-tag
branch
from
2c65d1d to
4ae5f99
Compare
Walkthrough映画一覧にタグ絞り込みが追加され、MoviesController#indexがタグコンテキスト（ Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Browser
participant MoviesController
participant TagModel
participant MovieModel
participant JS
participant API
Note over User,Browser: ユーザが一覧またはタグページにアクセス
User->>Browser: GET /movies or /movies/tags/:tag
Browser->>MoviesController: request (params[:tag])
MoviesController->>TagModel: Tag.find_by(name: params[:tag]) (if present)
MoviesController->>MovieModel: Movie.by_tag(params[:tag]).includes(:user).order(...).page(...)
MovieModel-->>MoviesController: movies
MoviesController-->>Browser: render index (movies, tags, tag)
Note over Browser,JS: 管理者がタグ編集を開く
Browser->>JS: click "タグ名変更" → show modal
JS->>API: PATCH /api/tags/:id { tag: { name } }
API-->>JS: 200 OK
JS->>Browser: update URL to /movies/tags/:new_name (replace current tag page)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 RuboCop (1.76.1)app/components/tag/edit_component.rbrubocop-minitest extension supports plugin, specify app/controllers/movies_controller.rbrubocop-minitest extension supports plugin, specify config/routes.rbrubocop-minitest extension supports plugin, specify
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
app/controllers/movies_controller.rb (1)
11-18: 🛠️ Refactor suggestion
@moviesが nil になる可能性があります上記
by_tagの仕様に依存しているため、現状params[:tag]が無い通常の/moviesで 500 エラーになります。
モデル側を修正するか、コントローラ側でフォールバックする処理を追加してください。- @movies = Movie.by_tag(params[:tag]) + @movies = Movie.by_tag(params[:tag])（モデル修正が入ればこの行はそのままで可）
🧹 Nitpick comments (4)
app/controllers/movies_controller.rb (1)
12-13: タグ一覧取得の N+1 & 並び順
Movie.all.all_tagsは全ムービーをロードしてからタグを集計するためメモリ負荷が高いです。
ActsAsTaggableOn にはActsAsTaggableOn::Tag.for_context(:tags)を直接呼ぶ方法があります。加えてorder(:name)を付けると UI が安定します。app/views/movies/index.html.slim (2)
25-37: ヘッダが二重定義になっています既存
.page-main-headerに加えてタグ用ヘッダをもう一つ描画しているため、同一ページに同クラスの要素が 2 つ並びます。CSS の想定外挙動やアクセシビリティに影響するので、
・既存ヘッダを条件分岐で置き換える
・もしくはタグ用情報だけを既存ヘッダ内に差し込む
など構造を一本化してください。
49-61: タグ一覧リンクの表示順を安定化視認性向上のため
@tagsを名前昇順で並べ替えると UX が向上します。test/system/movie/tags_test.rb (1)
51-62: マージ後の件数ハードコードはフィクスチャ変更で壊れやすい
assert_text "タグ「#{new_tag.name}」の動画（2）"の2はタグ付け件数に依存するハードコードです。
今後フィクスチャや実装を変更すると値がずれテストが落ちるリスクがあります。件数ではなく、対象動画タイトルの一部を確認するなど、より堅牢なアサーションに置き換えると保守性が向上します。
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/controllers/movies_controller.rb(1 hunks)app/models/movie.rb(1 hunks)app/views/movies/index.html.slim(3 hunks)config/routes.rb(1 hunks)test/fixtures/acts_as_taggable_on/taggings.yml(1 hunks)test/system/movie/tags_test.rb(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build_and_test
🔇 Additional comments (4)
test/fixtures/acts_as_taggable_on/taggings.yml (1)
105-118: タグ付けフィクスチャの追加は問題なし
Movie へのタグ付与がテストに必要な最小限の行追加で済んでおり、他既存データとの衝突もありません。config/routes.rb (1)
122-122: ルート追加は妥当です
/movies/tags/:tagを先に定義しているため RESTfulresources :moviesとの衝突も回避されています。app/views/movies/index.html.slim (1)
2-5: meta description は OK
タグ有無でメタを分岐させる実装は問題ありません。test/system/movie/tags_test.rb (1)
18-25: Ajax 完了待ちが無い場合、テストが不安定になる可能性
click_on '保存'後に非同期リクエストが走る場合、即座にassert_text '追加タグ'を実行すると要素がまだ描画されておらず失敗するケースがあります。
assert_textは一定時間リトライしますが、処理が長い場合はassert_no_css('.is-loading')等でロード完了を明示的に待つと安定します。
必要に応じてご検討ください。
|
@thmz337 |
|
お疲れ様です！こちら現状把握も兼ねて動作確認を行っていたのですが、 
こちらは仕様になるのでしょうか。メンターレビューに進んでも良いかご確認いただけますと幸いです。 |
|
@thmz337 こちらは僕の方で修正しますー レビューは進めてくださいー |
|
@machida |
|
@thmz337 conflictの修正をお願いします。 |
thmz337
force-pushed
the
feature/add-list-of-movies-by-tag
branch
from
6d44136 to
be28986
Compare
|
@thmz337 コミットのところのアイコンがデフォルトになっているようです。 
|
thmz337
force-pushed
the
feature/add-list-of-movies-by-tag
branch
from
be28986 to
6f08986
Compare
修正しましたので、ご確認よろしくお願いいたします。 |
|
@thmz337 commit内容とメッセージが合っていないように思います。 
これですとこのPRでは具体的に実装した部分がないように見えちゃうかなと思います。 |
thmz337
force-pushed
the
feature/add-list-of-movies-by-tag
branch
from
6f08986 to
3563dc6
Compare
|
お疲れ様です。修正しましたので、ご確認よろしくお願いいたします。 |
app/views/movies/index.html.slim
Outdated
| .page-main-header__end | ||
| .page-main-header-actions | ||
| .page-main-header-actions__items | ||
| = react_component('Tags/TagEditButton', tagId: @tag.id, tagName: @tag.name) |
There was a problem hiding this comment.
Vue.jsとReactを外すIssueを他で進めているところなので極力新しいReactの部分は作らず、バニラJSで行きたい感じです。
There was a problem hiding this comment.
確認ありがとうございます。承知しました。
thmz337
force-pushed
the
feature/add-list-of-movies-by-tag
branch
from
89275df to
7128291
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/javascript/tag_edit.js (1)
67-69: エラーハンドリングの改善を検討してください現在はコンソールにエラーをログ出力するだけですが、ユーザーにエラーメッセージを表示することを検討してください。
.catch((error) => { console.warn(error) + alert('タグの更新に失敗しました。再度お試しください。') })
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
app/components/tag/edit_component.html.slim(1 hunks)app/components/tag/edit_component.rb(1 hunks)app/controllers/movies_controller.rb(1 hunks)app/javascript/packs/application.js(1 hunks)app/javascript/tag_edit.js(1 hunks)app/models/movie.rb(1 hunks)app/views/movies/index.html.slim(3 hunks)config/routes.rb(1 hunks)test/components/tag/edit_component_test.rb(1 hunks)test/fixtures/acts_as_taggable_on/taggings.yml(1 hunks)test/system/movie/tags_test.rb(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/javascript/packs/application.js
🚧 Files skipped from review as they are similar to previous changes (6)
- app/controllers/movies_controller.rb
- app/models/movie.rb
- config/routes.rb
- app/views/movies/index.html.slim
- test/fixtures/acts_as_taggable_on/taggings.yml
- test/system/movie/tags_test.rb
🧰 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/components/tag/edit_component.rbtest/components/tag/edit_component_test.rb
**/*.js
⚙️ CodeRabbit Configuration File
**/*.js: - どうしても避けられない時以外に新規にVue.js, Reactのコードを追加しない。
- fetchメソッドの代わりにrequest.jsを使う。
Files:
app/javascript/tag_edit.js
test/**/*
⚙️ CodeRabbit Configuration File
test/**/*: # Test
- TestCase名は英語で書く。
- どうしても避けられない時以外にsystem testでsleepは使わない。
Unit Test
model, helper, decorator, view_componentについてはメソッドを追加した場合は必ず対応したUnit TestのTestCaseを1つは書く。
Files:
test/components/tag/edit_component_test.rb
🧠 Learnings (2)
📓 Common learnings
Learnt from: ryufuta
PR: fjordllc/bootcamp#8992
File: app/models/correct_answer_notifier.rb:23-23
Timestamp: 2025-07-30T07:26:36.599Z
Learning: PRのスコープを維持することが重要であり、バグ修正PRにおいて、修正対象以外の機能（例：ルーティングテスト）の追加提案は適切ではない。そのような改善が必要な場合は別PRで対応すべきである。
📚 Learning: fjordllc/bootcampプロジェクトでは、hタグ（見出し）の文言は日本語でハードコーディングする方針が確立されており、i18n対応は行わない。例：app/views/welcome/logo...
Learnt from: jun-kondo
PR: fjordllc/bootcamp#0
File: :0-0
Timestamp: 2025-07-23T20:42:19.974Z
Learning: fjordllc/bootcampプロジェクトでは、hタグ（見出し）の文言は日本語でハードコーディングする方針が確立されており、I18n対応は行わない。例：app/views/welcome/logo.html.slimでh2、h3タグが日本語でハードコーディングされている。
Applied to files:
app/components/tag/edit_component.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 (7)
test/components/tag/edit_component_test.rb (1)
14-20: 基本的なテストケースは適切です
tag_idとtag_nameの属性が正しく公開されていることを確認するテストは適切です。シンプルなコンポーネントには十分なテストカバレッジです。app/components/tag/edit_component.rb (1)
3-10: ViewComponentパターンに従った適切な実装ですシンプルでわかりやすいコンポーネント実装になっています。
attr_readerを使用してデータを公開し、コンストラクタで必要な情報を抽出する設計は適切です。app/components/tag/edit_component.html.slim (2)
1-4: モーダルトリガーボタンの実装は適切ですアイコンと日本語ラベルを使用したボタンの実装は、プロジェクトのガイドラインに従って適切に作成されています。
15-19: データ属性の設定が適切ですJavaScriptとの連携に必要な
data-tag-idとdata-initial-tag-name属性が正しく設定されており、コンポーネントとの連携が適切に実装されています。app/javascript/tag_edit.js (3)
34-41: フォームバリデーションロジックが適切です入力値の変更に応じて保存ボタンの有効/無効を切り替える実装が適切に動作します。空文字列と初期値のチェックも正しく行われています。
52-66: API呼び出しの実装が適切ですCSRF トークンの設定、適切なHTTPヘッダー、JSON形式でのデータ送信など、セキュリティとベストプラクティスに従った実装になっています。
43-50: URL置換ロジックは現状のままで問題ありませんアプリ内のタグ一覧ページ（articles, pages, questions, movies, users など）はすべて
*/tags/:tagというパス構造を共有しており、location.pathname.replace( `/tags/${encodeURIComponent(initialTagName)}`, `/tags/${encodeURIComponent(tagName)}` )による文字列置換で正しく動作します。
– 将来的にページネーションやクエリパラメータを維持したい場合は、以下のように
location.searchを付与する実装を検討してください。const url = new URL(location.href); url.pathname = url.pathname.replace( `/tags/${encodeURIComponent(initialTagName)}`, `/tags/${encodeURIComponent(tagName)}` ); location.href = url.toString();Likely an incorrect or invalid review comment.
thmz337
force-pushed
the
feature/add-list-of-movies-by-tag
branch
from
ae84fa4 to
8e9e383
Compare
thmz337
force-pushed
the
feature/add-list-of-movies-by-tag
branch
from
8e9e383 to
c7c4993
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
test/system/movie/tags_test.rb (1)
6-15: 順序非依存の比較に修正済みで安定性向上
expected_titles.sortとactual_titles.sortの比較により、DBやビューの並びの差異で落ちない堅牢なテストになっています。良いです。
🧹 Nitpick comments (5)
app/views/movies/index.html.slim (2)
25-37: タグ見出しと管理用編集UIの挿入はOKだが、アクセシビリティの向上余地あり実装自体は妥当です。アクセシビリティ向上のため、現在表示中のタグ名を示す手がかり（例: 見出しのすぐ後に visually-hidden な補足）を検討しても良いです。
50-63: タグ一覧リンクに aria-current を付与して現在のタグを明示しよう現在選択中のタグがある場合、該当リンクに
aria-current="page"を付与すると支援技術で文脈が伝わりやすくなります。以下のような条件付き属性付与を検討してください。
- = link_to tag.name, movies_tag_path(tag.name), class: 'page-tags-nav__item-link' + = link_to tag.name, + movies_tag_path(tag.name), + class: 'page-tags-nav__item-link', + aria: (@tag&.name == tag.name ? { current: 'page' } : nil)test/system/movie/tags_test.rb (1)
64-71: 同値更新で変更ボタンがdisabledになる動作の検証は適切UI仕様どおりの検証で良いです。余裕があれば、空文字入力時もdisabledになることの検証を追加するとより堅牢になります。
app/javascript/tag_edit.js (2)
22-26: キャンセル時に保存ボタンのdisabledを戻すキャンセルで値は初期化されますが、
keyupが発火しないためボタン状態が更新されません。キャンセル時にdisabledへ戻してください。cancelButton?.addEventListener('click', (e) => { e.preventDefault() tagForm.value = initialTagName + if (tagSaveButton) tagSaveButton.disabled = true setShowModal(false) })
34-41: disabled属性の操作はプロパティを用いると簡潔で安全
setAttribute/removeAttributeではなく、disabledプロパティでのON/OFFが簡潔です。- if (tagValue === initialTagName || tagValue === '') { - tagSaveButton.setAttribute('disabled', true) - } else { - tagSaveButton.removeAttribute('disabled') - } + const disabled = tagValue === initialTagName || tagValue === '' + if (tagSaveButton) tagSaveButton.disabled = disabled
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
app/components/tag/edit_component.html.slim(1 hunks)app/components/tag/edit_component.rb(1 hunks)app/controllers/movies_controller.rb(1 hunks)app/javascript/packs/application.js(1 hunks)app/javascript/tag_edit.js(1 hunks)app/models/movie.rb(1 hunks)app/views/movies/index.html.slim(3 hunks)config/routes.rb(1 hunks)test/components/tag/edit_component_test.rb(1 hunks)test/fixtures/acts_as_taggable_on/taggings.yml(1 hunks)test/system/movie/tags_test.rb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- app/models/movie.rb
- app/javascript/packs/application.js
- app/components/tag/edit_component.rb
- test/components/tag/edit_component_test.rb
- app/components/tag/edit_component.html.slim
- app/controllers/movies_controller.rb
🧰 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:
config/routes.rbtest/system/movie/tags_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/fixtures/acts_as_taggable_on/taggings.ymltest/system/movie/tags_test.rb
**/*.js
⚙️ CodeRabbit Configuration File
**/*.js: - どうしても避けられない時以外に新規にVue.js, Reactのコードを追加しない。
- fetchメソッドの代わりにrequest.jsを使う。
Files:
app/javascript/tag_edit.js
🧠 Learnings (1)
📚 Learning: 2025-06-30T01:07:46.192Z
Learnt from: e-yanagita-gs
PR: fjordllc/bootcamp#8771
File: app/controllers/welcome_controller.rb:57-57
Timestamp: 2025-06-30T01:07:46.192Z
Learning: プロジェクトでは、仕様上データの存在が保証されている場合、nil処理を省略することがある。特にFAQ表示のような特定機能に特化したPRでは、データの存在を前提とした実装が採用される。
Applied to files:
app/views/movies/index.html.slim
🧬 Code Graph Analysis (1)
app/javascript/tag_edit.js (1)
app/javascript/components/Tags/TagEditModal.jsx (1)
tagName(6-6)
⏰ 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/fixtures/acts_as_taggable_on/taggings.yml (1)
112-126: 映画タグ付けフィクスチャ追加の確認完了
- test/fixtures/acts_as_taggable_on/tags.yml に
cat、game、beginnerが定義済み- test/fixtures/movies.yml に
movie1、movie2が定義済み- 他のフィクスチャ命名規則や ERB ブロック配置も問題なし
これにより、追加されたタグ付けフィクスチャは整合性が取れており、問題ありません。
Approveします。config/routes.rb (1)
123-123: タグ別動画一覧のルーティング追加は既存パターンに準拠しており問題なし
- pages_tag, questions_tag と同様に
tag: /.+/とformat: "html"を指定しており一貫性があります。resources :moviesより前に宣言されているため、意図した優先度でマッチします。app/views/movies/index.html.slim (1)
2-5: メタディスクリプションの条件分岐は適切タグ有無で説明文を出し分ける実装は適切です。XSS面でもSlimのエスケープが効く前提で問題はありません。
| test 'admin updates a tag to a new name' do | ||
| old_tag = acts_as_taggable_on_tags('beginner') | ||
| new_tag_name = '上級者' | ||
|
|
||
| visit_with_auth movies_tag_path(old_tag.name, all: 'true'), 'komagata' | ||
| click_button 'タグ名変更' | ||
| fill_in('tag[name]', with: new_tag_name) | ||
| click_button '変更' | ||
|
|
||
| assert_text "タグ「#{new_tag_name}」の動画（1）" | ||
| end |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
タグ名変更後のURLとクエリ維持を検証するアサーションを追加しよう
現状のJS実装では、タグ名変更後に location.pathname を直接置換しており、?all=true 等のクエリが失われる懸念があります。テストでURL維持を検証し、リグレッションを防ぎましょう。
以下のようなアサーション追加を検討ください。
visit_with_auth movies_tag_path(old_tag.name, all: 'true'), 'komagata'
click_button 'タグ名変更'
fill_in('tag[name]', with: new_tag_name)
click_button '変更'
assert_text "タグ「#{new_tag_name}」の動画（1）"
+ assert_current_path movies_tag_path(new_tag_name, all: 'true')以下のスクリプトでビューとJS実装を検索し、タグ名変更後のURL遷移処理を確認しましょう。
🏁 Script executed:
#!/bin/bash
# 'タグ名変更'を含むビューを検索
rg -n "タグ名変更" -n app/views
# location.pathname を直接操作している JS コードを検索
rg -n "location\.pathname" -n app/javascriptLength of output: 659
クエリパラメータを維持したURL遷移をテストでカバーしましょう
現在のJS実装では、タグ名変更後に
location.href = location.pathname.replace(…)
を使っているため、?all=true などのクエリが消えてしまいます。
リグレッションを防ぐため、システムテストにURL保持のアサーションを追加してください。
修正ポイント例：
- app/javascript/tag_edit.js（46行目付近）
- app/javascript/components/Tags/TagEditModal.jsx（16行目付近）
- test/system/movie/tags_test.rb
提案するテストの追加例：
visit_with_auth movies_tag_path(old_tag.name, all: 'true'), 'komagata'
click_button 'タグ名変更'
fill_in('tag[name]', with: new_tag_name)
click_button '変更'
assert_text "タグ「#{new_tag_name}」の動画（1）"
+ assert_current_path movies_tag_path(new_tag_name, all: 'true')📝 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.
| test 'admin updates a tag to a new name' do | |
| old_tag = acts_as_taggable_on_tags('beginner') | |
| new_tag_name = '上級者' | |
| visit_with_auth movies_tag_path(old_tag.name, all: 'true'), 'komagata' | |
| click_button 'タグ名変更' | |
| fill_in('tag[name]', with: new_tag_name) | |
| click_button '変更' | |
| assert_text "タグ「#{new_tag_name}」の動画（1）" | |
| end | |
| test 'admin updates a tag to a new name' do | |
| old_tag = acts_as_taggable_on_tags('beginner') | |
| new_tag_name = '上級者' | |
| visit_with_auth movies_tag_path(old_tag.name, all: 'true'), 'komagata' | |
| click_button 'タグ名変更' | |
| fill_in('tag[name]', with: new_tag_name) | |
| click_button '変更' | |
| assert_text "タグ「#{new_tag_name}」の動画（1）" | |
| assert_current_path movies_tag_path(new_tag_name, all: 'true') | |
| end |
🤖 Prompt for AI Agents
In test/system/movie/tags_test.rb around lines 39 to 49, the system test
exercises renaming a tag but does not assert that query parameters (e.g.
?all=true) are preserved after the redirect; add an assertion after the rename
that the browser URL still contains the original query param (for example assert
current_url.include?('?all=true') or assert_match(/\?all=true$/,
URI.parse(current_url).query.to_s)) so the test fails if the JS change drops
query strings; keep the rest of the test same.
There was a problem hiding this comment.
こちらについて、現在の実装で問題ないことを確認しました。
There was a problem hiding this comment.
@thmz337 承知いたしました。実装を確認していただき、ありがとうございます。現在の実装で問題ないとのことですので、追加のアサーションは不要ですね。
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/javascript/tag_edit.js (1)
7-14: tagEditModal/tagForm が存在しない場合の null 参照クラッシュ（早期リターンで防止）
tagEditModalが見つからないケースや、モーダル内に対象の入力要素がないケースでquerySelector/dataset参照時に例外になります。早期 return でガードしてください。以下のように修正を提案します。
- const tagEditModal = document.querySelector('.edit-tag-modal') - const tagForm = tagEditModal.querySelector('.a-text-input') + const tagEditModal = document.querySelector('.edit-tag-modal') + if (!tagEditModal) return + const tagForm = tagEditModal.querySelector('.a-text-input') + if (!tagForm) return const cancelButton = tagEditModal.querySelector('.cancel-change-tag-button') const tagSaveButton = tagEditModal.querySelector('.save-tag-button') const initialTagName = tagForm.dataset.initialTagName const tagId = tagForm.dataset.tagId
🧹 Nitpick comments (3)
app/javascript/tag_edit.js (3)
36-43: 保存ボタンが存在しない DOM に対するガード（軽微な堅牢化）
keyupハンドラ内でtagSaveButtonを直接操作しているため、ボタンが見つからない DOM 構造だと例外になります。optional chaining で安全に扱いましょう。- if (tagValue === initialTagName || tagValue === '') { - tagSaveButton.setAttribute('disabled', true) - } else { - tagSaveButton.removeAttribute('disabled') - } + if (tagValue === initialTagName || tagValue === '') { + tagSaveButton?.setAttribute('disabled', true) + } else { + tagSaveButton?.removeAttribute('disabled') + }
30-34: クリック連打対策と入力値の前後空白除去（UX/堅牢性の向上）保存時に前後空白を除去し、通信中は二重送信を防ぐためボタンを無効化するのが無難です。
updateTagをawaitし、最小限のガードを入れておくと事故を防げます。- tagSaveButton?.addEventListener('click', async (e) => { + tagSaveButton?.addEventListener('click', async (e) => { e.preventDefault() - const tagName = tagForm.value - updateTag(tagName) + const tagName = tagForm.value.trim() + tagSaveButton.setAttribute('disabled', true) + try { + await updateTag(tagName) + } finally { + // 遷移する場合は実行されませんが、保険として + tagSaveButton.removeAttribute('disabled') + } })
45-52: 履歴を汚したくない場合は location.replace の利用を検討相対パスで
location.hrefを代入してもクエリ/ハッシュは保持される点は把握済み（学習メモの通り）なので実装はそのままでOKです。そのうえで、保存後の戻る履歴を増やしたくない場合はlocation.replace(...)を使うと UX が向上します。- location.href = location.pathname.replace( + location.replace(location.pathname.replace( `/tags/${encodeURIComponent(initialTagName)}`, `/tags/${encodeURIComponent(tagName)}` - ) + ))補足: 既存のクエリ/ハッシュは相対遷移で維持されます（本PRのスレッドでの検証・学習に基づく前提）。
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/javascript/tag_edit.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.js
⚙️ CodeRabbit Configuration File
**/*.js: - どうしても避けられない時以外に新規にVue.js, Reactのコードを追加しない。
- fetchメソッドの代わりにrequest.jsを使う。
Files:
app/javascript/tag_edit.js
🧠 Learnings (1)
📚 Learning: 2025-08-12T15:34:46.372Z
Learnt from: thmz337
PR: fjordllc/bootcamp#8590
File: app/javascript/tag_edit.js:46-49
Timestamp: 2025-08-12T15:34:46.372Z
Learning: JavaScriptで `location.href` に相対パスを代入した場合、ブラウザは自動的に現在のクエリパラメータ（search）とハッシュフラグメント（hash）を保持する。`location.pathname.replace(...)` の結果を `location.href` に代入しても、クエリやハッシュは失われない。
Applied to files:
app/javascript/tag_edit.js
🧬 Code Graph Analysis (1)
app/javascript/tag_edit.js (1)
app/javascript/components/Tags/TagEditModal.jsx (1)
tagName(6-6)
⏰ 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/tag_edit.js (1)
1-1: request.js の採用はガイドライン準拠で良いですfetch ではなく
@rails/request.jsのpatchを使っている点、チーム方針に合致しています。
| const updateTag = async (tagName) => { | ||
| await patch(`/api/tags/${tagId}`, { | ||
| body: JSON.stringify({ tag: { name: tagName } }), | ||
| contentType: 'application/json' | ||
| }) | ||
| .then(() => { | ||
| updateTagList(tagName) | ||
| }) | ||
| .catch((error) => { | ||
| console.warn(error) | ||
| }) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
レスポンスの成否判定と await/then 混在解消（エラー時に遷移しない）
現状は patch のレスポンス成否を見ずに常に updateTagList を呼ぶため、422 等の失敗でも画面遷移してしまいます。また await と .then() が混在しています。res.ok を確認して成功時のみ遷移、try/catch に一本化しましょう。
- const updateTag = async (tagName) => {
- await patch(`/api/tags/${tagId}`, {
- body: JSON.stringify({ tag: { name: tagName } }),
- contentType: 'application/json'
- })
- .then(() => {
- updateTagList(tagName)
- })
- .catch((error) => {
- console.warn(error)
- })
- }
+ const updateTag = async (tagName) => {
+ try {
+ const res = await patch(`/api/tags/${tagId}`, {
+ body: JSON.stringify({ tag: { name: tagName } }),
+ contentType: 'application/json'
+ })
+ if (res && res.ok) {
+ updateTagList(tagName)
+ } else {
+ console.warn('Failed to update tag:', res?.status, res?.statusText)
+ }
+ } catch (error) {
+ console.warn(error)
+ }
+ }📝 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 updateTag = async (tagName) => { | |
| await patch(`/api/tags/${tagId}`, { | |
| body: JSON.stringify({ tag: { name: tagName } }), | |
| contentType: 'application/json' | |
| }) | |
| .then(() => { | |
| updateTagList(tagName) | |
| }) | |
| .catch((error) => { | |
| console.warn(error) | |
| }) | |
| } | |
| const updateTag = async (tagName) => { | |
| try { | |
| const res = await patch(`/api/tags/${tagId}`, { | |
| body: JSON.stringify({ tag: { name: tagName } }), | |
| contentType: 'application/json' | |
| }) | |
| if (res && res.ok) { | |
| updateTagList(tagName) | |
| } else { | |
| console.warn('Failed to update tag:', res?.status, res?.statusText) | |
| } | |
| } catch (error) { | |
| console.warn(error) | |
| } | |
| } |
🤖 Prompt for AI Agents
In app/javascript/tag_edit.js around lines 54–65, the function mixes await with
.then()/.catch() and always calls updateTagList even if the PATCH failed; change
it to a single async/await flow inside a try/catch, await the patch call into a
response variable, check response.ok and call updateTagList(tagName) only on
success, and in the catch or non-ok branch log or surface the error and do not
navigate/update the UI.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
app/javascript/tag_edit.js (2)
7-11: モーダル/入力要素が存在しない場合の null 参照をガード（再指摘）
.edit-tag-modalやその子要素が存在しないとquerySelectorチェーンで例外になります。早期 return で防ぎましょう。以下の差分を適用してください。
- const tagEditModal = document.querySelector('.edit-tag-modal') - const tagForm = tagEditModal.querySelector('.a-text-input') - const cancelButton = tagEditModal.querySelector('.cancel-change-tag-button') - const tagSaveButton = tagEditModal.querySelector('.save-tag-button') + const tagEditModal = document.querySelector('.edit-tag-modal') + if (!tagEditModal) return + const tagForm = tagEditModal.querySelector('.a-text-input') + if (!tagForm) return + const cancelButton = tagEditModal.querySelector('.cancel-change-tag-button') + const tagSaveButton = tagEditModal.querySelector('.save-tag-button')
54-65: await と then の混在/レスポンス未検査により失敗時も遷移してしまうPATCH が 4xx/5xx でも常に
updateTagListが呼ばれてしまいます。async/await に統一しres.okを確認して成功時のみ更新してください。入力も送信前に trim しましょう。- const updateTag = async (tagName) => { - await patch(`/api/tags/${tagId}`, { - body: JSON.stringify({ tag: { name: tagName } }), - contentType: 'application/json' - }) - .then(() => { - updateTagList(tagName) - }) - .catch((error) => { - console.warn(error) - }) - } + const updateTag = async (tagName) => { + const name = tagName.trim() + if (!name) return + try { + const res = await patch(`/api/tags/${tagId}`, { + body: JSON.stringify({ tag: { name } }), + contentType: 'application/json' + }) + if (res?.ok) { + updateTagList(name) + } else { + console.warn('Failed to update tag:', res?.status, res?.statusText) + } + } catch (error) { + console.warn(error) + } + }任意で、保存押下時に二重送信防止のため一時的に保存ボタンを disabled にする対応も検討ください。
🧹 Nitpick comments (3)
app/javascript/tag_edit.js (3)
19-23: モーダル表示時に入力へフォーカスを移動（UX向上）開いた瞬間に入力できるようにフォーカスを当てると操作性が上がります。
tagChangeButton?.addEventListener('click', (e) => { e.preventDefault() setShowModal(true) + tagForm.focus() })
36-43: 保存ボタンの活性/非活性判定に trim を適用し、null セーフに前後空白のみの入力を弾き、要素が欠けていても落ちないようにしておくと堅牢です。
- tagForm?.addEventListener('keyup', (e) => { - const tagValue = e.target.value - if (tagValue === initialTagName || tagValue === '') { - tagSaveButton.setAttribute('disabled', true) - } else { - tagSaveButton.removeAttribute('disabled') - } - }) + tagForm?.addEventListener('keyup', (e) => { + const tagValue = e.target.value.trim() + const initial = initialTagName?.trim?.() ?? initialTagName + if (tagValue === '' || tagValue === initial) { + tagSaveButton?.setAttribute('disabled', true) + } else { + tagSaveButton?.removeAttribute('disabled') + } + })加えて、初期表示時にも同じ判定を適用したい場合は、上記ロジックを関数化して DOM 構築後に一度呼び出すのがよいです。
45-52: 履歴を汚さずに遷移したい場合はlocation.replaceを検討現在の実装でも動作はしますが、戻る操作で古い URL に戻らせたくない場合は
replaceが適しています。なお、クエリ/ハッシュ保持の件は先の検証結果（相対パス代入で保持される）に従って本件は問題ありません。- location.href = location.pathname.replace( - `/tags/${encodeURIComponent(initialTagName)}`, - `/tags/${encodeURIComponent(tagName)}` - ) + location.replace( + location.pathname.replace( + `/tags/${encodeURIComponent(initialTagName)}`, + `/tags/${encodeURIComponent(tagName)}` + ) + )
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/javascript/tag_edit.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.js
⚙️ CodeRabbit Configuration File
**/*.js: - どうしても避けられない時以外に新規にVue.js, Reactのコードを追加しない。
- fetchメソッドの代わりにrequest.jsを使う。
Files:
app/javascript/tag_edit.js
🧠 Learnings (1)
📚 Learning: 2025-08-12T15:34:46.372Z
Learnt from: thmz337
PR: fjordllc/bootcamp#8590
File: app/javascript/tag_edit.js:46-49
Timestamp: 2025-08-12T15:34:46.372Z
Learning: JavaScriptで `location.href` に相対パスを代入した場合、ブラウザは自動的に現在のクエリパラメータ（search）とハッシュフラグメント（hash）を保持する。`location.pathname.replace(...)` の結果を `location.href` に代入しても、クエリやハッシュは失われない。
Applied to files:
app/javascript/tag_edit.js
🧬 Code Graph Analysis (1)
app/javascript/tag_edit.js (1)
app/javascript/components/Tags/TagEditModal.jsx (1)
tagName(6-6)
⏰ 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/tag_edit.js (1)
3-3: turbo:load へのフックは不要です (Turbo/Turbolinks 未使用)
リポジトリ内を以下の手順で確認したところ、
@hotwired/turbo-railsやturbolinksの import- ビュー内の
data-turbo/data-turbolinks属性
いずれも検出されませんでした。
そのため、DOMContentLoadedへのみフックする実装で問題ありません。
|
お疲れ様です。動画タグの修正フォームを、 ReactからVanillaJSに移行しました。 以上よろしくお願いいたします🙇♀️ |
thmz337
force-pushed
the
feature/add-list-of-movies-by-tag
branch
from
5f99935 to
a96c96d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
app/models/movie.rb (2)
25-26: after_create_commit におけるon: :createは冗長（かつ無視される）
after_create_commitは「create時のcommit後」に限定された専用マクロのため、on: :createオプションは不要です（ActiveSupportのコールバックオプション正規化でも利用されず読み手の混乱要因）。削ってシンプルにしてください。- after_create_commit :start_transcode_job, on: :create + after_create_commit :start_transcode_job
29-34: after_commit内での再スローはUX劣化の恐れ（保存成功後でも500になり得る）
after_create_commitはトランザクション確定後に実行されます。ここで例外を再スローすると、レコード保存は成功しているのにリクエストは500で落ちる可能性があります（キューアダプタの一時障害等）。非同期処理の起動失敗はログ＋エラートラッキングに留め、リクエストは成功させる運用が実務的です。検討案A（推奨: 例外は握りつぶし、監視に委譲）
def start_transcode_job TranscodeJob.perform_later(self) -rescue StandardError => e - Rails.logger.error("Failed to enqueue TranscodeJob for Movie #{id}: #{e.message}") - raise +rescue StandardError => e + Rails.logger.error("Failed to enqueue TranscodeJob for Movie #{id}: #{e.class}: #{e.message}") + # 監視サービスがあれば通知する（例：Sentry/Rails.error.report など） + Rails.error.report(e, context: { movie_id: id }) if Rails.respond_to?(:error) + nil end検討案B（厳格運用: ジョブ起動失敗は保存も失敗扱いにしたい）
- after_create_commit ではなく before/after_save +
throw(:abort)を使う、もしくは明示的にサービス層で「保存+ジョブ登録」をトランザクションにまとめて扱う（今回の要件ではAが妥当と判断）。どちらの方針がプロダクトポリシーに合うか確認お願いします。
app/views/movies/index.html.slim (3)
2-5: メタディスクリプションの文言はi18n化を検討SEO文言がハードコードされています。将来の文言調整や多言語対応を見据えて
config/locales/ja.ymlへ退避を推奨します。キー例：views.movies.index.meta.description_with_tag/views.movies.index.meta.description_default。
25-37: ヘッダーが二重化しているためレイアウト崩れ再発の懸念あり上部に既存の「動画」ヘッダー（Line 11-24）があり、タグ選択時に二つ目の
.page-main-headerを追加しています。ブレークポイントによっては高さ・余白計算や「被り」問題が再発しやすい構成です。既存ヘッダー内のタイトル・アクション領域を条件分岐で切替える構成へ寄せると安全です。
- 想定案：既存ヘッダー内の
h1を@tag.present? ? "タグ「#{@tag.name}」の動画（#{@movies.total_count}）" : "動画"に切替、管理者時の編集コンポーネントは.page-main-header__endに条件表示。- 可能であればデザイン責務の影響が小さいコンポーネント（ViewComponent）化も検討。
スタイル修正済とのことですが、以下の解像度で再確認をお願いします: 1280px, 1440px, 1920px。
50-62: 現在選択中タグの可視化とa11y改善（aria-current, 選択スタイル）タグナビに現在選択中のタグを視覚的・音声的に示すとUXが向上します。
aria-current="page"と選択クラス付与を提案します。また、タグリンクであることを明示するrel="tag"も付与可能です。- = link_to tag.name, movies_tag_path(tag.name), class: 'page-tags-nav__item-link' + = link_to tag.name, + movies_tag_path(tag.name), + class: ['page-tags-nav__item-link', (@tag&.name == tag.name ? 'is-current' : nil)].compact.join(' '), + aria: { current: (@tag&.name == tag.name ? 'page' : nil) }, + rel: 'tag'補足:
- 「すべて」を選択状態にできるよう、未選択時のナビ上部に
movies_pathへの「すべて」リンクを追加するのも有効です（任意）。app/controllers/movies_controller.rb (2)
11-12: params未指定時の無駄なDBヒット回避
params[:tag]が空のときでもfind_by(name: nil)を叩くため軽微ですが無駄なクエリになります。presenceガードを。- @tag = ActsAsTaggableOn::Tag.find_by(name: params[:tag]) + @tag = ActsAsTaggableOn::Tag.find_by(name: params[:tag]) if params[:tag].present?
13-17: 一覧クエリはOK。タグ一覧取得の負荷をモニタリング推奨
Movie.by_tag(...).includes(:user).order(...).page(...).per(...)の組み合わせは問題ありません。
一方、@tags = Movie.all.all_tagsはデータ量によっては重くなる可能性があるため、キャッシュ（例：フラグメントキャッシュ or タグの最終更新時刻ベース）か、用途に応じて「使用数の多い順・上位N件」などの絞り込みを検討してください。
- まずはログ/メトリクスでレスポンスタイムと発行SQLを確認してボトルネックかを検証するのがよいです。
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
app/components/tag/edit_component.html.slim(1 hunks)app/components/tag/edit_component.rb(1 hunks)app/controllers/movies_controller.rb(1 hunks)app/javascript/packs/application.js(1 hunks)app/javascript/tag_edit.js(1 hunks)app/models/movie.rb(1 hunks)app/views/movies/index.html.slim(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- app/components/tag/edit_component.rb
- app/javascript/tag_edit.js
- app/javascript/packs/application.js
- app/components/tag/edit_component.html.slim
🧰 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/movies_controller.rbapp/models/movie.rb
🧠 Learnings (3)
📚 Learning: 2025-07-12T09:56:13.360Z
Learnt from: thmz337
PR: fjordllc/bootcamp#8590
File: app/models/movie.rb:22-24
Timestamp: 2025-07-12T09:56:13.360Z
Learning: Rails scopeでnilやfalseを返すと、NoMethodErrorは発生せず、代わりにModel.allが返される。これによりメソッドチェインは継続するが、全レコードが読み込まれるため意図しない動作となる可能性がある。
Applied to files:
app/models/movie.rb
📚 Learning: 2025-06-30T01:07:46.192Z
Learnt from: e-yanagita-gs
PR: fjordllc/bootcamp#8771
File: app/controllers/welcome_controller.rb:57-57
Timestamp: 2025-06-30T01:07:46.192Z
Learning: プロジェクトでは、仕様上データの存在が保証されている場合、nil処理を省略することがある。特にFAQ表示のような特定機能に特化したPRでは、データの存在を前提とした実装が採用される。
Applied to files:
app/models/movie.rbapp/views/movies/index.html.slim
📚 Learning: 2025-07-07T12:46:01.650Z
Learnt from: masyuko0222
PR: fjordllc/bootcamp#8277
File: test/models/searcher_test.rb:0-0
Timestamp: 2025-07-07T12:46:01.650Z
Learning: テストにおいて、`result.all? { |condition| }` でコレクションの全要素が条件を満たすことを確認できれば、その条件を満たさない要素が含まれていないことも論理的に保証されるため、追加のアサーションは冗長となる。
Applied to files:
app/models/movie.rb
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build_and_test
🔇 Additional comments (1)
app/models/movie.rb (1)
23-23: by_tagスコープは意図が明確でOK（常にRelationを返す）
tag.present? ? tagged_with(tag) : allにより常にActiveRecord::Relationを返す実装になっており、チェーン安全性が担保されています。以前の議論点（nil時にallへフォールバックする暗黙挙動への依存）も回避できています。
thmz337
force-pushed
the
feature/add-list-of-movies-by-tag
branch
from
a96c96d to
b049bf5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/models/movie.rb (1)
23-23: by_tagは常にActiveRecord::Relationを返すよう明示しましょう（過去コメントの再提示）
tagが未指定/空のときにnilを返すと、Rails の仕様で暗黙にallへフォールバックします。動作はたまたま期待どおりでも、意図がコードに表れず将来のリスクになります。present?判定で明示的にallを返す方が安全です。以前のスレッドで合意されていた修正が現状のコードには反映されていないようなので、再度提案します。適用diff:
- scope :by_tag, ->(tag) { tagged_with(tag) if tag } + scope :by_tag, ->(tag) { tag.present? ? tagged_with(tag) : all }
🧹 Nitpick comments (4)
app/components/tag/edit_component.rb (1)
6-10: nil想定が不要ならそのままでOK／必要なら引数ガードを追加本プロジェクトでは「仕様上存在が保証されるデータはnil処理を省略」する方針があるため（学習メモ参照）、このままでも問題ありません。一方で将来的に呼び出し元が増える可能性を考慮するなら、軽い引数ガードを入れて早期に失敗させるとデバッグ性が上がります。
例（任意）:
def initialize(tag:) + raise ArgumentError, "tag is required" if tag.nil? @tag_id = tag.id @tag_name = tag.name endapp/controllers/movies_controller.rb (1)
11-17: タグ一覧取得方法とタグN+1回避のオプショナルリファクタ提案– 現状、
Movie.all.all_tagsは acts_as_taggable_on gem が提供するメソッドですが、内部実装によっては大量レコードをロードしてしまう可能性があります。標準 API のMovie.tags_on(:tags)（件数不要）や、件数情報も合わせて取得したい場合はMovie.tag_counts_on(:tags)の利用を検討してください。
– ビュー内でmovie.tag_list.eachを呼び出しているため、タグ取得時にレコードごとに追加クエリが発行される可能性があります。コントローラの@moviesに対してincludes(:tags)を追加し、タグ関連を事前ロードすることで N+1 問題を防止できます。対象箇所
- app/controllers/movies_controller.rb（11–17行）
- app/views/movies/_movie_list.html.slim（38–40行、
movie.tag_list.each）提案diff
--- a/app/controllers/movies_controller.rb +++ b/app/controllers/movies_controller.rb @@ -11,7 +11,9 @@ class MoviesController < ApplicationController @tag = ActsAsTaggableOn::Tag.find_by(name: params[:tag]) - @tags = Movie.all.all_tags + # タグ一覧（件数不要） + @tags = Movie.tags_on(:tags) + # 件数情報も必要なら → Movie.tag_counts_on(:tags) @movies = Movie.by_tag(params[:tag]) - .includes(:user) + .includes(:user, :tags) .order(updated_at: :desc, id: :desc) .page(params[:page]) .per(PAGER_NUMBER)app/components/tag/edit_component.html.slim (1)
26-28: キャンセル要素はbutton要素にしてキーボード操作可能に現在は
div相当のノードです。インタラクティブ要素はbuttonにすることで、Enter/Space で操作でき、デフォルトのフォーカス管理やアクセシビリティが改善されます。- .cancel-change-tag-button.card-main-actions__muted-action - | キャンセル + button.cancel-change-tag-button.card-main-actions__muted-action type="button" + | キャンセルtest/components/tag/edit_component_test.rb (1)
14-21: レンダリング結果の属性も検証して回 regress を防ぎましょう現在は
attr_readerの値のみをテストしています。テンプレートのfor属性やdata-*属性が意図どおり出力されるかも合わせて検証すると、今回のlabel属性のような不具合を早期検出できます。例（追記テスト案）:
def test_template_wires_label_and_input doc = Capybara.string(rendered_content) input = doc.find('input#tag_name', visible: :all) label = doc.find('label.a-form-label[for="tag_name"]', visible: :all) assert_equal @tag.name, input[:value] assert_equal @tag.id.to_s, input['data-tag-id'] assert_equal @tag.name, input['data-initial-tag-name'] end
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
app/components/tag/edit_component.html.slim(1 hunks)app/components/tag/edit_component.rb(1 hunks)app/controllers/movies_controller.rb(1 hunks)app/javascript/packs/application.js(1 hunks)app/javascript/tag_edit.js(1 hunks)app/models/movie.rb(1 hunks)app/views/movies/index.html.slim(3 hunks)config/routes.rb(1 hunks)test/components/tag/edit_component_test.rb(1 hunks)test/fixtures/acts_as_taggable_on/taggings.yml(1 hunks)test/system/movie/tags_test.rb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- app/javascript/packs/application.js
- test/system/movie/tags_test.rb
- config/routes.rb
- app/javascript/tag_edit.js
- test/fixtures/acts_as_taggable_on/taggings.yml
- app/views/movies/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:
app/models/movie.rbapp/controllers/movies_controller.rbtest/components/tag/edit_component_test.rbapp/components/tag/edit_component.rb
test/**/*
⚙️ CodeRabbit configuration file
test/**/*: # Test
- TestCase名は英語で書く。
- どうしても避けられない時以外にsystem testでsleepは使わない。
Unit Test
model, helper, decorator, view_componentについてはメソッドを追加した場合は必ず対応したUnit TestのTestCaseを1つは書く。
Files:
test/components/tag/edit_component_test.rb
🧠 Learnings (3)
📚 Learning: 2025-07-12T09:56:13.360Z
Learnt from: thmz337
PR: fjordllc/bootcamp#8590
File: app/models/movie.rb:22-24
Timestamp: 2025-07-12T09:56:13.360Z
Learning: Rails scopeでnilやfalseを返すと、NoMethodErrorは発生せず、代わりにModel.allが返される。これによりメソッドチェインは継続するが、全レコードが読み込まれるため意図しない動作となる可能性がある。
Applied to files:
app/models/movie.rb
📚 Learning: 2025-06-30T01:07:46.192Z
Learnt from: e-yanagita-gs
PR: fjordllc/bootcamp#8771
File: app/controllers/welcome_controller.rb:57-57
Timestamp: 2025-06-30T01:07:46.192Z
Learning: プロジェクトでは、仕様上データの存在が保証されている場合、nil処理を省略することがある。特にFAQ表示のような特定機能に特化したPRでは、データの存在を前提とした実装が採用される。
Applied to files:
app/models/movie.rb
📚 Learning: 2025-07-07T12:46:01.650Z
Learnt from: masyuko0222
PR: fjordllc/bootcamp#8277
File: test/models/searcher_test.rb:0-0
Timestamp: 2025-07-07T12:46:01.650Z
Learning: テストにおいて、`result.all? { |condition| }` でコレクションの全要素が条件を満たすことを確認できれば、その条件を満たさない要素が含まれていないことも論理的に保証されるため、追加のアサーションは冗長となる。
Applied to files:
app/models/movie.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
|
上記の他にも複数の対応が行われていない指摘があるようです。やりとりをお願いします。 |
thmz337
force-pushed
the
feature/add-list-of-movies-by-tag
branch
from
b049bf5 to
c4acebd
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
config/routes.rb (1)
125-125: ルート順序の確認結果
config/routes.rb 上で
get "movies/tags/:tag"が125行目resources :moviesが136行目
に定義されており、タグ用ルートはresources :moviesよりも前に置かれています。現状問題ありません。CIで順序入れ替えを検知する仕組みは、必要に応じて後ほど検討してください。System Test のエッジケース追加提案
test/system/movie/tags_test.rb には基本的なタグ検索テストが含まれていますが、以下のようなエッジケースもカバーしておくとリグレッション防止に役立ちます。
- 「C++」や「a.b」のようにピリオド・プラス記号を含むタグ
- 「日本語タグ」などマルチバイト文字を含むタグ
これらを1〜2件、
movies_tag_pathを使った visit テストとして追加しておくことをおすすめします。app/javascript/packs/application.js (1)
87-87: （任意）必要ページのみ動的 import にしてバンドルを軽量化
tag_edit.jsが映画一覧やタグ編集 UI に限定されるなら、DOM 存在確認後の動的 import にすると初期ロードをわずかに削減できます。既存のグローバル読込方針を崩したくない場合はスルーでOKです。対象行の差分（静的 import を外す）:
- import '../tag_edit.js'DOM 監視での遅延読込例（既存の DOMContentLoaded ハンドラ内に追記）:
// 例: 既存の DOMContentLoaded の中に追記 document.addEventListener('DOMContentLoaded', () => { // ...既存処理... const hasTagEdit = document.querySelector('[data-component="tag-edit"], .js-tag-edit'); if (hasTagEdit) { import('../tag_edit.js'); } });
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
app/components/tag/edit_component.html.slim(1 hunks)app/components/tag/edit_component.rb(1 hunks)app/controllers/movies_controller.rb(1 hunks)app/javascript/packs/application.js(1 hunks)app/javascript/tag_edit.js(1 hunks)app/models/movie.rb(1 hunks)app/views/movies/index.html.slim(3 hunks)config/routes.rb(1 hunks)test/components/tag/edit_component_test.rb(1 hunks)test/fixtures/acts_as_taggable_on/taggings.yml(1 hunks)test/system/movie/tags_test.rb(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- test/fixtures/acts_as_taggable_on/taggings.yml
🚧 Files skipped from review as they are similar to previous changes (8)
- app/components/tag/edit_component.rb
- test/components/tag/edit_component_test.rb
- test/system/movie/tags_test.rb
- app/controllers/movies_controller.rb
- app/javascript/tag_edit.js
- app/views/movies/index.html.slim
- app/components/tag/edit_component.html.slim
- app/models/movie.rb
🧰 Additional context used
📓 Path-based instructions (2)
**/*.js
⚙️ CodeRabbit configuration file
**/*.js: - どうしても避けられない時以外に新規にVue.js, Reactのコードを追加しない。
- fetchメソッドの代わりにrequest.jsを使う。
Files:
app/javascript/packs/application.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:
config/routes.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)
config/routes.rb (1)
125-125: タグ絞り込みルートの追加は既存パターンと整合していて妥当です
pages_tag/questions_tagと同じ構造（/movies/tags/:tag+format: "html"+tag: /.+/）で統一できており、ビューからmovies_tag_path(tag.name)を使いやすい命名です。app/javascript/packs/application.js (2)
87-87: tag_edit.js の読込追加は既存パックの方針に沿っており問題ありません
tag.jsに隣接して編集用tag_edit.jsを取り込む配置は妥当です。副作用ベースの初期化が多いパック構成に一貫しています。
87-87: 確認完了:@rails/request.js利用および副作用スコープは要件を満たしています
import { patch } from '@rails/request.js'のインポートを確認済みfetch()の直接呼び出しは存在せず、すべて@rails/request.js経由でリクエストを実行DOMContentLoaded内で要素取得後にif (!…) returnによる存在チェックを行い、対象要素がないページで副作用が発生しないよう制御以上のとおり問題ありませんので、このままマージしてください。
|
お疲れ様です。ご確認ありがとうございます。
こちら申し訳ございません。やり取りを行いましたので、ご確認よろしくお願いいたします。 |
| @@ -0,0 +1,74 @@ | |||
| import { patch } from '@rails/request.js' | |||
5 participants
Issue
概要
下記を追加しました。
動画機能は本番環境にデプロイされていますが、まだ全機能が実装されておらず使用段階ではないため隠匿されています。なので、動作確認の際は直接URLを入力してページを開いてください。
変更確認方法
feature/add-list-of-movies-by-tagをローカルに取り込むScreenshot
動画一覧ページ
タグで絞り込んだページ
Summary by CodeRabbit
新機能
バグ修正
テスト