Conversation
Walkthrough複数のフロントエンド実装（Rails ViewComponent、React、Vue）と関連テスト・従来の初期化スクリプトを削除し、DOM参照から動的に初期化するクラスベースの Changes
Sequence Diagram(s)sequenceDiagram
participant ProductJS as Product.jsx
participant DOM as DOM Element\n(data-product-id)
participant Checker as product-checker.js\n(ProductChecker)
participant API as /api/products/checker
ProductJS->>DOM: レンダリング (data-属性付き div)
ProductJS->>Checker: dynamic import -> new ProductChecker(el)
Checker->>Checker: constructor() / initProductChecker()
alt 未割当 or 自分が担当
Checker->>DOM: generateActionButton() を表示
else 他ユーザーが担当
Checker->>DOM: generateAssigneeDisplay() を表示
end
Note over DOM,Checker: ユーザがボタンをクリック
DOM->>Checker: handleCheckerToggle()
Checker->>Checker: isAssigned を切替、method 決定(PATCH/DELETE)
Checker->>API: checkProduct(productId,currentUserId,url,method)
API-->>Checker: レスポンス (checker 情報)
Checker->>DOM: updateButton/再描画
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 分 多数のフロントエンドパラダイム（React/Vue/ViewComponent）を横断して削除・差し替えし、API 呼び出しラッパーの置換や動的インポートの導入があるため、挙動確認と依存整合性検証にやや時間がかかります。 Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (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: 4
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
app/components/products/product_checker_component.html.slim(0 hunks)app/components/products/product_checker_component.rb(0 hunks)app/javascript/checkable.js(0 hunks)app/javascript/components/Product.jsx(2 hunks)app/javascript/components/ProductChecker.jsx(0 hunks)app/javascript/packs/application.js(0 hunks)app/javascript/product-checker.js(1 hunks)app/javascript/product_checker.js(0 hunks)app/javascript/product_checker_button.js(0 hunks)test/components/product/product_checker_component_test.rb(0 hunks)
💤 Files with no reviewable changes (8)
- app/javascript/packs/application.js
- test/components/product/product_checker_component_test.rb
- app/components/products/product_checker_component.html.slim
- app/javascript/components/ProductChecker.jsx
- app/javascript/checkable.js
- app/components/products/product_checker_component.rb
- app/javascript/product_checker.js
- app/javascript/product_checker_button.js
🧰 Additional context used
📓 Path-based instructions (1)
**/*.js
⚙️ CodeRabbit configuration file
**/*.js: - どうしても避けられない時以外に新規にVue.js, Reactのコードを追加しない。
- fetchメソッドの代わりにrequest.jsを使う。
Files:
app/javascript/product-checker.js
🧬 Code graph analysis (2)
app/javascript/components/Product.jsx (1)
app/javascript/product-checker.js (1)
ProductChecker(4-86)
app/javascript/product-checker.js (1)
app/javascript/new-comment.js (1)
currentUserId(33-33)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build_and_test
🔇 Additional comments (5)
app/javascript/product-checker.js (3)
16-16: 型の不一致に注意してください。
checkerId === currentUserIdの比較で型の不一致が発生する可能性があります。データ属性は文字列として取得されるため、片方が数値の場合、厳密等価演算子では一致しません。型を統一するか、必要に応じて型変換を行ってください:
- this.isSelfAssigned = checkerId === currentUserId + this.isSelfAssigned = String(checkerId) === String(currentUserId)
55-61: 実装は適切です。ボタンの生成とイベントリスナーの登録が正しく行われています。
63-85: 実装は適切です。担当者表示のDOM生成ロジックは正しく実装されています。
app/javascript/components/Product.jsx (2)
29-32: 動的インポートは良いアプローチです。コード分割のために動的インポートを使用しているのは良い判断です。
97-103: データ属性の実装は適切です。チェッカー要素のデータ属性が正しく設定されており、ProductChecker クラスで使用されるすべての必要な情報が含まれています。
| updateButton(button, isAssigned) { | ||
| button.innerHTML = '' | ||
| const config = isAssigned | ||
| ? { class: 'is-warning', icon: 'fa-times', label: '担当から外れる' } | ||
| : { class: 'is-secondary', icon: 'fa-hand-paper', label: '担当する' } | ||
| button.className = `a-button is-block ${config.class} is-sm` | ||
| button.textContent = config.label | ||
| const icon = document.createElement('i') | ||
| icon.className = `fas ${config.icon}` | ||
| button.appendChild(icon) | ||
| } |
There was a problem hiding this comment.
重大なバグ: ボタンのテキストが消失します。
29行目で button.innerHTML = '' でクリアした後、34行目で button.textContent = config.label を設定し、その後37行目で button.appendChild(icon) を実行していますが、textContent を設定するとボタンの子要素がすべて削除されるため、アイコンのみが表示され、ラベルテキストが消失します。
以下の修正を適用してください:
updateButton(button, isAssigned) {
button.innerHTML = ''
const config = isAssigned
? { class: 'is-warning', icon: 'fa-times', label: '担当から外れる' }
: { class: 'is-secondary', icon: 'fa-hand-paper', label: '担当する' }
button.className = `a-button is-block ${config.class} is-sm`
- button.textContent = config.label
const icon = document.createElement('i')
icon.className = `fas ${config.icon}`
+ button.textContent = config.label
button.appendChild(icon)
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In app/javascript/product-checker.js around lines 28 to 38, the code clears the
button with button.innerHTML = '' and then sets button.textContent =
config.label before appending the icon, which causes the label to be removed
when DOM operations occur; instead, clear existing children (or keep the clear),
then create a text node for config.label and append it before appending the icon
so both label and icon remain visible: remove/keep the initial innerHTML clear
as needed, create const labelNode = document.createTextNode(config.label) and
call button.appendChild(labelNode) prior to button.appendChild(icon), keeping
the className assignment intact.
There was a problem hiding this comment.
button.textContent = config.labelはbutton.appendChild(icon)より前に実行しており、実際にテキストとアイコンが両方正しく表示されています。
現状の実装で問題ないと考えいます。
認識あっていますでしょうか？
There was a problem hiding this comment.
@sjabcdefin さん、ご指摘ありがとうございます。
申し訳ございません。私の分析が誤っていました。おっしゃる通り、この実装で問題ありません。
button.textContent = config.label でテキストノードが作成された後、button.appendChild(icon) でアイコン要素が追加されるため、ボタンには [テキストノード] [アイコン要素] の両方が正しく表示されます。
混乱を招いてしまい申し訳ありませんでした。現状の実装で正しく動作します。🎉
| handleCheckerToggle(button) { | ||
| const nextIsAssigned = !this.isAssigned | ||
| this.updateButton(button, nextIsAssigned) | ||
|
|
||
| checkProduct( | ||
| this.productId, | ||
| this.currentUserId, | ||
| '/api/products/checker', | ||
| this.isAssigned ? 'DELETE' : 'PATCH', | ||
| CSRF.getToken() | ||
| ) | ||
|
|
||
| this.isAssigned = nextIsAssigned | ||
| } |
There was a problem hiding this comment.
エラーハンドリングが不足しています。
checkProduct API呼び出しが失敗した場合、UIの状態がサーバーの状態と不整合になります。現在、42行目でUIを更新し、44-50行目でAPI呼び出しを行い、52行目でローカル状態を更新していますが、APIエラー時の処理がありません。
API呼び出しを try-catch で囲み、エラー時にUIを元の状態に戻すことを推奨します:
handleCheckerToggle(button) {
const nextIsAssigned = !this.isAssigned
this.updateButton(button, nextIsAssigned)
- checkProduct(
- this.productId,
- this.currentUserId,
- '/api/products/checker',
- this.isAssigned ? 'DELETE' : 'PATCH',
- CSRF.getToken()
- )
+ try {
+ await checkProduct(
+ this.productId,
+ this.currentUserId,
+ '/api/products/checker',
+ this.isAssigned ? 'DELETE' : 'PATCH',
+ CSRF.getToken()
+ )
+ this.isAssigned = nextIsAssigned
+ } catch (error) {
+ // API呼び出しが失敗した場合、UIを元に戻す
+ this.updateButton(button, this.isAssigned)
+ console.error('Failed to toggle checker:', error)
+ }
+}
- this.isAssigned = nextIsAssigned
-}注: この変更には handleCheckerToggle を async にする必要があります。
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In app/javascript/product-checker.js around lines 40 to 53, the UI is updated
optimistically before the API call and there is no error handling, so UI and
server state can diverge if checkProduct fails; make handleCheckerToggle async,
await the checkProduct call inside a try-catch, and in catch revert the button
UI and this.isAssigned to their previous values (and optionally log or show the
error) so the UI reflects the real server state on failure.
There was a problem hiding this comment.
checkProductのAPI実行に失敗してもボタンの文言は即座には元に戻りませんが、ページを再読み込みすれば正しいchecker_idに応じた文言に更新されます。
そのため、現状のようにcheckProduct内で try/catch して、console.warnやトーストでエラーを通知する仕様で十分かと思っています。
ご意見いただけますでしょうか？
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the implementation of checkProduct in checkable_react.js
fd -t f "checkable_react" --exec cat {}
Length of output: 846
@sjabcdefin さん、ご説明ありがとうございます。
おっしゃる通り、checkProduct 内で既にエラーハンドリングが実装されており、トーストやコンソールでエラーが通知される仕様になっていますね。また、ページリロード時に正しい状態に同期される点も確認しました。
現在の実装では checkProduct が Promise を返していないため、私の提案を実装するには checkProduct 自体をリファクタリングする必要があり、変更範囲が大きくなってしまいます。
一時的な状態の不整合というUX上の懸念はありますが、エラー通知が適切に表示される前提であれば、現在のシンプルなアプローチで十分だと思います。了解しました。
🧬 Code Graph Analysis Results
<filename: app/javascript/new-comment.js, lines: 33-33>
const currentUserId = newComment.dataset.current_user_id
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
app/javascript/product-checker.js (1)
53-59: フォーム誤送信防止: ボタンtype指定 と 連打防止（任意）
- フォーム内に配置される可能性を考慮し、生成ボタンに
type="button"を付与してください（デフォルトsubmit回避）。- 任意ですが、API呼出し中は
disabledで連打防止するとUXが安定します（失敗時のUIロールバックは過去議論を尊重し提案しません）。generateActionButton() { const button = document.createElement('button') + button.type = 'button' this.updateButton(button, this.isAssigned) this.element.appendChild(button) - button.addEventListener('click', () => this.handleCheckerToggle(button)) + button.addEventListener('click', () => { + button.disabled = true + Promise.resolve( + this.handleCheckerToggle(button) + ).finally(() => { + button.disabled = false + }) + }) }
🧹 Nitpick comments (3)
app/javascript/components/Product.jsx (1)
99-105: *data-属性とdatasetキーの整合性OK（任意でref化も可）
data-checker-id等→dataset.checkerId等へのマッピングは整合しています。任意ですが、document.querySelector依存を減らすため、この要素にrefを付与してそのままupdateProductCheckerView(ref.current)に渡す実装も検討可（パフォーマンスと可読性の微改善）。app/javascript/product-checker.js (2)
15-16: 型差異に強い自己割当判定へ
datasetは文字列になるため、checkerId === currentUserIdは型差異でfalseになる可能性があります。安全のため文字列比較へ寄せてください。- this.isSelfAssigned = checkerId === currentUserId + this.isSelfAssigned = String(checkerId) === String(currentUserId)
69-75: ニトピック: alt文言のローカライズ
'Checker Avatar'は日本語UIでは不自然です。例:'担当者アイコン'やthis.checkerNameを含めた代替テキストにするとアクセシビリティが向上します。- img.alt = 'Checker Avatar' + img.alt = this.checkerName ? `${this.checkerName}のアイコン` : '担当者アイコン'
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/javascript/checkable_react.js(1 hunks)app/javascript/components/Product.jsx(2 hunks)app/javascript/product-checker.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.js
⚙️ CodeRabbit configuration file
**/*.js: - どうしても避けられない時以外に新規にVue.js, Reactのコードを追加しない。
- fetchメソッドの代わりにrequest.jsを使う。
Files:
app/javascript/product-checker.jsapp/javascript/checkable_react.js
🧬 Code graph analysis (2)
app/javascript/product-checker.js (1)
app/javascript/checkable_react.js (2)
checkProduct(4-31)checkProduct(4-31)
app/javascript/components/Product.jsx (1)
app/javascript/product-checker.js (1)
ProductChecker(3-84)
⏰ 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/components/Product.jsx (1)
29-33: 動的importと初期化の流れは妥当（nullガードOK）
updateProductCheckerViewの動的import→初期化、およびquerySelector後のnullチェックは適切です。前回指摘のnullガードも反映済みでLGTMです。Also applies to: 35-42
app/javascript/checkable_react.js (1)
6-12: 致命的: bodyがJSON化されていない／response.jsonの呼び出しミス
- FetchRequestに渡す
bodyがプレーンオブジェクトのままです。Fetchにそのまま渡るためTypeErrorになります。JSON.stringify(...)とContent-Type: application/jsonを付与してください。const body = await response.jsonは関数参照をawaitしており常に不正です。await response.json()が正です。- 可能なら
contentTypeを確認してからjson()を呼ぶと安全です（非JSONレスポンスでの例外回避）。As per coding guidelines / Based on learnings適用例:
import { FetchRequest } from '@rails/request.js' export const checkProduct = async (productId, currentUserId, url, method) => { try { const request = new FetchRequest(method, url, { redirect: 'manual', - body: { - product_id: productId, - current_user_id: currentUserId - } + headers: { + 'Content-Type': 'application/json', + 'Accept': 'application/json' + }, + body: JSON.stringify({ + product_id: productId, + current_user_id: currentUserId + }) }) - const response = await request.perform() - const body = await response.json + const response = await request.perform() + const isJson = response.contentType?.includes('application/json') + const body = isJson ? await response.json() : null if (response.ok) { - if (body.checker_id !== null) { + if (body && body.checker_id !== null) { toast('担当になりました。') } else { toast('担当から外れました。') } } else { - if (body.message) { - toast(body.message, 'error') - } + if (body?.message) { + toast(body.message, 'error') + } else { + toast('更新に失敗しました。もう一度お試しください。', 'error') + } } } catch (error) { console.warn(error) } }Also applies to: 14-16, 17-27
⛔ Skipped due to learnings
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でない場合は自動的にエラーを投げる安全性チェックも含まれている。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#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#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はgetterとして定義されており、それぞれPromiseを返す。jsongetterは自動的にContent-Typeをチェックし、application/jsonでない場合はエラーを投げる。また、レスポンスは一度パースされるとキャッシュされる仕組みになっている。
|
@kitarou888 |
|
@sjabcdefin |
|
@kitarou888 他の方にレビュー依頼してみようと思います。😌🙏 |
|
@sjabcdefin tomoka さんの環境では私が「レビュー依頼なし」のステータスになっていたとのことでしたら（Fjord Choiceの不具合？）こちらぜひやらせていただきます！ 
|
|
@kitarou888 すみません！ |
kitarou888
left a comment
kitarou888
left a comment
There was a problem hiding this comment.
@sjabcdefin
レビューが遅くなりすみませんでした🙇♂️
動作、コードともに気になる点はありませんでしたので私からはApproveとさせていただきます！
※以下、良いと感じた点をFBとして書かせていただきます👍
- 概要欄にDiffの根拠が書かれている点
- useEffectに使用した理由が書かれている点
|
@kitarou888 @komagata |
3 participants
Issue
概要
app/javascript/product-checker.jsを作成(使用されていない同名のファイルがあったため置き換え)app/javascript/components/Product.jsxのProductChecker呼び出し部分を修正app/javascript/components/ProductChecker.jsxを削除ProductChecker関連のファイルを削除しました。app/javascript/checkable_react.jsのfetchをrequest.jsに置き換えました。変更確認方法
chore/replace-product-checker-vanilla-jsをローカルに取り込むforeman start -f Procfile.devでアプリを起動するkomagataさんでログインし、以下のページでそれぞれ確認するページ
提出物一覧での表示確認
「担当する」「担当から外れる」の動作確認
Screenshot
見た目の変更はありませんが、確認時の参考のために添付いたします。
Summary by CodeRabbit
リファクタリング
テスト