ActionCompletedButtonComponentを再利用可能にする #9331
Conversation
WalkthroughActionCompletedButtonComponentの初期化引数を Changes
Sequence Diagram(s)sequenceDiagram
participant Browser
participant JS as action_completed_button.js
participant API as API::TalksController
participant DB as Database
Browser->>JS: ボタンクリック
JS-->>JS: isLoading=true, ボタン無効化
JS->>API: PATCH update_path (body: { modelName: { action_completed } })
API->>DB: talk.update!(params) (validation & save)
DB-->>API: 保存成功
API-->>JS: 204 No Content
JS-->>Browser: UI更新（ボタン文言・アイコン・説明、トースト）
JS-->>JS: isLoading=false, ボタン有効化
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 分
Possibly related issues
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/javascript/action_completed_button.js (1)
4-17: 将来の複数ボタン対応を見越すならquerySelectorAll＋ ボタン単位の状態にしておくと安心です現状
.check-buttonは1つだけ存在する前提でquerySelectorとモジュールスコープのisLoadingを使っており、このPRの要件は満たしています。ただ、同一ページに複数の「対応済にするボタン」を置きたくなった場合、先頭の1つしか動かない形になるので、再利用性を少し上げるならボタンごとにハンドラとisLoadingを持たせる実装にしておくと拡張が楽だと思います。例えば次のような形です:
-document.addEventListener('DOMContentLoaded', () => { - const button = document.querySelector('.check-button') - if (!button) { - return - } - - let isLoading = false; - const updatePath = button.dataset.updatePath - const modelName = button.dataset.modelName - button.addEventListener('click', async () => { - if (isLoading) return - - isLoading = true - button.disabled = true +document.addEventListener('DOMContentLoaded', () => { + const buttons = document.querySelectorAll('.check-button') + if (!buttons.length) { + return + } + + buttons.forEach((button) => { + let isLoading = false + const updatePath = button.dataset.updatePath + const modelName = button.dataset.modelName + + button.addEventListener('click', async () => { + if (isLoading) return + + isLoading = true + button.disabled = true @@ 以降のクリックハンドラ内部は現行ロジックのまま - isLoading = false - button.disabled = false - }) + isLoading = false + button.disabled = false + }) + }) })現状の要件では必須ではないので、余裕があるタイミングでの対応で十分だと思います。
Also applies to: 22-27, 54-56
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/components/action_completed_button_component.html.slim(2 hunks)app/components/action_completed_button_component.rb(1 hunks)app/controllers/api/talks_controller.rb(1 hunks)app/javascript/action_completed_button.js(2 hunks)app/views/talks/show.html.slim(1 hunks)test/components/action_completed_button_component_test.rb(1 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
app/**/*.{rb,js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Rails app code should be organized in
app/directory with subdirectories:models/,controllers/,views/,jobs/,helpers/, and frontend code underjavascript/(Shakapacker)
Files:
app/components/action_completed_button_component.rbapp/controllers/api/talks_controller.rbapp/javascript/action_completed_button.js
**/*.rb
📄 CodeRabbit inference engine (AGENTS.md)
Ruby code should use 2-space indentation, snake_case for method names, and CamelCase for class names, enforced by RuboCop (
.rubocop.yml)
Files:
app/components/action_completed_button_component.rbapp/controllers/api/talks_controller.rbtest/components/action_completed_button_component_test.rb
⚙️ CodeRabbit configuration file
**/*.rb: # refactoring
- まずFat Controllerを避け、次にFat Modelを避ける。
- Serviceクラスの乱用を避ける。
- controller concernを作ろうとしたらPORO(Plain Old Ruby Object)やActiveRecordモデルでの実装で代替できないか検討する。
Rails Patterns
- ViewHelperにメソッドを実装する時にはまずDecoratorパターンを使うことを検討する。（active_decorator gemを導入しているのでそれを使う）
- 複雑なActiveRecordクエリがあり、再利用できそうな場合はQueryObjectパターンを検討する。（rails-patterns gemを導入しているのでそれのQuery機能を使う）
- Viewにpartialを作る場合はViewComponentを使うことを検討する。
- 複数のActiveRecordモデルを操作する１つの責務がある時や外部APIとやりとりする処理がある場合にはInteractorオブジェクトパターンを検討する。（interactor gemを導入しているのでそれを使う）
- 複数のInteractorを実行するような処理がある場合Organizerオブジェクトパターンを検討する。（interactor gemを導入しており、その中にOrganizerの機能があるので使う）
Files:
app/components/action_completed_button_component.rbapp/controllers/api/talks_controller.rbtest/components/action_completed_button_component_test.rb
**/*.slim
📄 CodeRabbit inference engine (AGENTS.md)
Slim templates should be linted according to
config/slim_lint.yml
Files:
app/components/action_completed_button_component.html.slimapp/views/talks/show.html.slim
app/javascript/**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
JavaScript/TypeScript code in
app/javascript/should be linted with ESLint and formatted with Prettier viayarn lintscripts; use React 17 and Shakapacker/Webpack 5
Files:
app/javascript/action_completed_button.js
**/*.js
⚙️ CodeRabbit configuration file
**/*.js: - どうしても避けられない時以外に新規にVue.js, Reactのコードを追加しない。
- fetchメソッドの代わりにrequest.jsを使う。
Files:
app/javascript/action_completed_button.js
test/**/*_test.rb
📄 CodeRabbit inference engine (AGENTS.md)
test/**/*_test.rb: Test suite should use Minitest with structure:system/,models/,controllers/, and fixtures intest/fixtures/; test files should be named*_test.rb
Use Minitest + Capybara for system tests; place unit and integration tests under matchingtest/*directories
Files:
test/components/action_completed_button_component_test.rb
test/**
📄 CodeRabbit inference engine (AGENTS.md)
Keep tests deterministic and use fixtures stored in
test/fixtures/for test data
Files:
test/components/action_completed_button_component_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/components/action_completed_button_component_test.rb
🧠 Learnings (3)
📓 Common learnings
Learnt from: tyrrell-IH
Repo: fjordllc/bootcamp PR: 9306
File: app/javascript/components/Bookmarks.jsx:248-265
Timestamp: 2025-11-17T00:46:30.794Z
Learning: fjordllc/bootcamp プロジェクトでは、Reactからバニラ JavaScript への移行作業が進行中である。そのため、新しいReactファイルの作成は避け、既存のReactコンポーネント内で完結する実装が推奨される。BookmarksInDashboard.jsx は issue #9045 で削除予定。
Learnt from: komagata
Repo: fjordllc/bootcamp PR: 9101
File: app/notifiers/discord_notifier.rb:131-135
Timestamp: 2025-09-12T21:18:00.834Z
Learning: Rails アップグレードPRにおいて、product_review_not_completed メソッドの webhook URL 設定の shared フォールバック追加も、設定システム全体の変更として別PRで対応すべきである。
Learnt from: komagata
Repo: fjordllc/bootcamp PR: 9101
File: app/models/graduation_notifier.rb:23-28
Timestamp: 2025-09-12T21:16:47.639Z
Learning: Rails upgrade PRにおいて、configuration systemの変更やwebhook設定の改善提案も、アップグレードに直接関連しない場合は別PRで対応すべきである。PRのスコープ維持が重要。
Learnt from: sjabcdefin
Repo: fjordllc/bootcamp PR: 9090
File: app/javascript/user-sns.js:2-25
Timestamp: 2025-08-25T08:00:11.369Z
Learning: app/javascript/user-sns.js において、アクセシビリティ改善（aria-label属性の追加など）は他の箇所との整合性を保つため、別途統一して対応する方針である
Learnt from: Miya096jp
Repo: fjordllc/bootcamp PR: 9102
File: app/controllers/mentor/buzzes_controller.rb:0-0
Timestamp: 2025-09-04T01:39:22.261Z
Learning: fjordllc/bootcamp プロジェクトでは、更新処理の成功時の通知メッセージについて、I18n を使用せずに日本語文字列を直接記述する方針で一貫性を保っている。
Learnt from: ryufuta
Repo: fjordllc/bootcamp PR: 9155
File: app/controllers/api/correct_answers_controller.rb:20-22
Timestamp: 2025-09-15T07:39:39.350Z
Learning: API::CorrectAnswersController#update では、ベストアンサー取り下げという単純な状態変更のため、update! を使用して DB エラー時に 500 を返し、成功時はデフォルトレスポンスを使用するのが適切。クライアント側のバリデーションエラーは発生しにくく、明示的なエラーハンドリングは不要。
📚 Learning: 2025-09-15T07:39:39.350Z
Learnt from: ryufuta
Repo: fjordllc/bootcamp PR: 9155
File: app/controllers/api/correct_answers_controller.rb:20-22
Timestamp: 2025-09-15T07:39:39.350Z
Learning: API::CorrectAnswersController#update では、ベストアンサー取り下げという単純な状態変更のため、update! を使用して DB エラー時に 500 を返し、成功時はデフォルトレスポンスを使用するのが適切。クライアント側のバリデーションエラーは発生しにくく、明示的なエラーハンドリングは不要。
Applied to files:
app/controllers/api/talks_controller.rb
📚 Learning: 2025-09-04T01:39:22.261Z
Learnt from: Miya096jp
Repo: fjordllc/bootcamp PR: 9102
File: app/controllers/mentor/buzzes_controller.rb:0-0
Timestamp: 2025-09-04T01:39:22.261Z
Learning: fjordllc/bootcamp プロジェクトでは、更新処理の成功時の通知メッセージについて、I18n を使用せずに日本語文字列を直接記述する方針で一貫性を保っている。
Applied to files:
app/javascript/action_completed_button.js
🔇 Additional comments (5)
app/components/action_completed_button_component.html.slim (1)
7-8: data属性の追加でコンポーネントの再利用性がきれいに担保されています
data-update-path/data-model-nameが両方のボタンに付与されており、JS側のdataset.updatePath/dataset.modelNameとも整合しているので、他画面からも同じコンポーネントをそのまま使い回しやすい構造になっていて良いと思います。Also applies to: 22-23
app/controllers/api/talks_controller.rb (1)
10-11: 状態更新APIとしてupdate!＋204 No Contentの構成は妥当です単純な
action_completedのトグルであればupdate!にしてDBエラー時のみ500に任せる実装としつつ、成功時はhead :no_contentでボディなし204を返す構成で問題ないと思います。API::CorrectAnswersController の方針とも揃っており、一貫した設計になっていて良さそうです。Based on learnings, ...test/components/action_completed_button_component_test.rb (1)
1-18: 完了/未完了の両状態をカバーするViewComponentテストが追加されていて良いです
is_initial_action_completedの true/false それぞれで期待テキストを検証しており、同時にupdate_path/model_nameをコンストラクタ引数に含めることで、新しいインターフェースが壊れていないことも担保できていて良いと思います。app/views/talks/show.html.slim (1)
65-65: ActionCompletedButtonComponent 呼び出しのインターフェース更新が他レイヤーと整合しています
api_talk_path(@talk)とmodel_name: 'talk'の組み合わせにより、JS側の{ talk: { action_completed } }というリクエストボディとtalk_paramsの strong parameters がちょうど一致しており、コンポーネントの再利用化に向けた最初の呼び出し箇所として問題ないと思います。app/components/action_completed_button_component.rb (1)
4-12: コンストラクタの引数整理とprivateなreader化がシンプルで扱いやすいです
is_initial_action_completed/update_path/model_nameを明示的なキーワード引数にしつつ、内部では@is_action_completedなどにマッピングし、Viewからはprivateなreader経由でしか触れないようにしてあるのは責務がはっきりしていて良いと思います。HTMLテンプレート側の呼び出しとも齟齬がなく、インターフェース変更がきれいにまとまっています。
CodeRabbitの指摘を受けて修正：#9331 (comment)
typoもあった
`app/components/action_completed_button_component.html.slim`からはprivateでも参照可能
ryufuta
force-pushed
the
chore/make-action-completed-button-reusable
branch
from
3484fba to
77163f5
Compare
|
@ryufuta まずComponentを再利用可能にしてからやるのいいですね。そしてそれを別Issueとするのもとってもいいですね。 |
Issue
概要
相談部屋の「対応済にするボタン」とお問い合わせ個別ページの「対応済にするボタン」は全く同じ機能・見た目だが、前者はViewComponentとバニラJSで、後者はReactで実装されている。
後者を非React化する上記のIssueに取り組むにあたって、本PRでは前者を再利用可能にした。
後者の非React化は本PRのマージ後に別のPRで、お問い合わせ機能の設計変更とともに実施する。
本PRの変更内容は以下。
※ #8535 (comment) にある通り、「対応済にするボタン」は今後、助成金コースの申請や研修の申し込みにも利用する予定のため、もともと再利用できるように実装する必要があった。
変更確認方法
chore/make-action-completed-button-reusableをローカルに取り込む参考：#8488 (comment)
Screenshot
見た目の変更はありません。
Summary by CodeRabbit
リリースノート
新機能
改善
テスト
✏️ Tip: You can customize this high-level summary in your review settings.