Conversation
WalkthroughReports.jsx が直接レンダリングしていた React 実装の PracticeFilterDropdown をプレースホルダ要素に置き換え、useEffect で動的 import して新規のプレーンJS クラス Changes
Sequence Diagram(s)sequenceDiagram
participant R as Reports.jsx (React)
participant E as useEffect
participant P as PracticeFilterDropdown (plain JS)
participant C as Choices.js
participant D as DOM
R->>E: マウント / data & practices 確認
E->>E: dynamic import('.../practice-filter-dropdown.js')
E->>P: new PracticeFilterDropdown(practices, selectedPracticeId, setSelectedPracticeId)
E->>P: render(target[data-practice-filter-dropdown])
P->>D: <select> と <option> を生成して挿入
P->>C: Choices.js を初期化（ローカライズ等）
Note over D,P: ユーザが選択を変更
D->>P: change イベント -> setPracticeId(selectedId)
P->>R: setSelectedPracticeId 呼び出し（親 state 更新）
Note over R,E: アンマウントまたは依存変更時
R->>E: cleanup
E->>P: destroy()
P->>C: Choices インスタンスとリスナを破棄
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related issues
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
💤 Files with no reviewable changes (1)
🧰 Additional context used📓 Path-based instructions (3)app/**/*.{rb,js,ts,jsx,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
app/javascript/**/*.{js,ts,jsx,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.js⚙️ CodeRabbit configuration file
Files:
🧠 Learnings (19)📓 Common learnings📚 Learning: 2025-11-13T20:44:13.908ZApplied to files:
📚 Learning: 2025-06-29T03:44:15.179ZApplied to files:
📚 Learning: 2025-07-23T20:42:19.974ZApplied to files:
📚 Learning: 2025-09-04T01:39:22.261ZApplied to files:
📚 Learning: 2025-11-17T00:46:30.794ZApplied to files:
📚 Learning: 2025-09-12T21:16:47.639ZApplied to files:
📚 Learning: 2025-07-30T07:26:36.599ZApplied to files:
📚 Learning: 2025-09-11T14:47:53.256ZApplied to files:
📚 Learning: 2025-09-12T21:18:00.834ZApplied to files:
📚 Learning: 2025-09-05T03:33:31.659ZApplied to files:
📚 Learning: 2025-11-19T07:55:51.504ZApplied to files:
📚 Learning: 2025-09-05T03:33:31.659ZApplied to files:
📚 Learning: 2025-08-25T07:23:54.802ZApplied to files:
📚 Learning: 2025-09-05T03:16:53.387ZApplied to files:
📚 Learning: 2025-08-31T03:39:07.792ZApplied to files:
📚 Learning: 2025-09-01T22:31:57.345ZApplied to files:
📚 Learning: 2025-09-01T22:31:57.345ZApplied to files:
📚 Learning: 2025-08-25T08:00:11.369ZApplied to files:
🧬 Code graph analysis (1)app/javascript/components/Reports.jsx (1)
⏰ 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)
🔇 Additional comments (3)
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 (2)
app/javascript/practice-filter-dropdown.js (1)
41-49:allowHTML: trueの必要性を確認してくださいセキュリティの観点から、
allowHTML: trueは本当に必要な場合のみ使用すべきです。現在のコードではtextContentを使用しているため、XSSリスクは低いですが、念のため確認をお願いします。app/javascript/components/Reports.jsx (1)
35-47: 動的インポートのエラーハンドリングを追加することを推奨します動的インポートが失敗した場合のエラーハンドリングがありません。ネットワークエラーやモジュールの読み込み失敗に対応するため、try-catchブロックの追加を検討してください。
例：
const initPracticeFilterDropdown = async () => { try { const target = document.querySelector('[data-practice-filter-dropdown]') if (!target) return const { default: PracticeFilterDropdown } = await import( '../practice-filter-dropdown' ) practiceFilterDropdownInstance = new PracticeFilterDropdown( practices, setUserPracticeId, userPracticeId ) practiceFilterDropdownInstance.render(target) } catch (error) { console.error('Failed to load PracticeFilterDropdown:', error) } }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/javascript/components/Reports.jsx(3 hunks)app/javascript/practice-filter-dropdown.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.js
⚙️ CodeRabbit configuration file
**/*.js: - どうしても避けられない時以外に新規にVue.js, Reactのコードを追加しない。
- fetchメソッドの代わりにrequest.jsを使う。
Files:
app/javascript/practice-filter-dropdown.js
🧬 Code graph analysis (1)
app/javascript/components/Reports.jsx (1)
app/javascript/practice-filter-dropdown.js (1)
PracticeFilterDropdown(3-67)
⏰ 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/practice-filter-dropdown.js (2)
3-9: クラス構造は適切ですコンストラクターで必要なプロパティが適切に初期化されています。
11-40: DOM構築は適切です
textContentを使用することでXSSリスクを適切に回避しています。app/javascript/components/Reports.jsx (2)
1-1: インポートの追加は適切です
73-73: プレースホルダー要素の配置は適切です動的にレンダリングされるコンポーネントのターゲット要素として適切に機能します。
Also applies to: 79-79
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/javascript/components/Reports.jsx (1)
75-75: プレースホルダー要素の配置を統一することを検討してください同じセレクター
data-practice-filter-dropdownを持つ要素が2つの異なる条件分岐（Line 75とLine 81）に存在します。現在はquerySelectorが最初にマッチした要素を返すため動作しますが、保守性の観点から、プレースホルダーを単一の場所に配置することを推奨します。以下のようにリファクタリングを検討してください：
return ( <div className="page-main is-react"> + {practices && <div data-practice-filter-dropdown></div>} {data.totalPages === 0 && ( <div> - {practices && <div data-practice-filter-dropdown></div>} <NoReports unchecked={unchecked} /> </div> )} {data.totalPages > 0 && ( <div> - {practices && <div data-practice-filter-dropdown></div>} <div className="page-body">Also applies to: 81-81
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/javascript/components/Reports.jsx(3 hunks)app/javascript/practice-filter-dropdown.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/javascript/practice-filter-dropdown.js
🧰 Additional context used
🧬 Code graph analysis (1)
app/javascript/components/Reports.jsx (1)
app/javascript/practice-filter-dropdown.js (1)
PracticeFilterDropdown(3-78)
⏰ 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/components/Reports.jsx (1)
1-1: 適切なインポートです
useEffectが正しくインポートされており、動的ロードのライフサイクル管理に使用されています。
karlley
force-pushed
the
chore/convert-practice-filter-dropdown-to-vanilla-js
branch
from
b9f053c to
da98019
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/javascript/components/PracticeFilterDropdown.jsx(0 hunks)app/javascript/components/Reports.jsx(3 hunks)app/javascript/practice-filter-dropdown.js(1 hunks)
💤 Files with no reviewable changes (1)
- app/javascript/components/PracticeFilterDropdown.jsx
🚧 Files skipped from review as they are similar to previous changes (1)
- app/javascript/components/Reports.jsx
🧰 Additional context used
📓 Path-based instructions (1)
**/*.js
⚙️ CodeRabbit configuration file
**/*.js: - どうしても避けられない時以外に新規にVue.js, Reactのコードを追加しない。
- fetchメソッドの代わりにrequest.jsを使う。
Files:
app/javascript/practice-filter-dropdown.js
🧠 Learnings (1)
📓 Common learnings
Learnt from: hirokiej
Repo: fjordllc/bootcamp PR: 8740
File: app/helpers/reports_helper.rb:56-64
Timestamp: 2025-06-29T03:44:15.179Z
Learning: このプロジェクト（fjordllc/bootcamp）では、ja.ymlファイルで一部の単語や文章のみI18n対応されているが、多くのテキストは日本語でハードコーディングされており、完全な国際化対応は行われていない。新しい機能でもI18n対応は不要と判断される。
🧬 Code graph analysis (1)
app/javascript/practice-filter-dropdown.js (1)
app/javascript/components/Reports.jsx (1)
userPracticeId(21-21)
⏰ 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/practice-filter-dropdown.js (2)
65-75: 適切なクリーンアップ実装です！イベントリスナーの削除、Choices.jsインスタンスの破棄、参照のクリアが正しく実装されています。過去のレビューで指摘されたメモリリークの問題が適切に修正されています。
43-51:allowHTML: trueは削除するか、必要性を確認してくださいLine 39で
option.textContentを使用しているため直接的なXSS injection は防がれていますが、allowHTML: trueによりChoices.jsのドロップダウン内でpractice.titleがHTMLとしてレンダリングされる可能性があります。
practice.titleはmentor/adminの入力のみに制限されていますが、防御的見地から以下のいずれかを推奨します：
allowHTML: trueを削除（プレーンテキストのタイトルレンダリングには不要）- または
practice.titleに対するHTML サニタイゼーションを追加
|
@kutimiti1234 |
|
@karlley |
|
はい、急いでいませんので全然大丈夫です！ |
|
@karlley さん |
|
@kutimiti1234
問題ないです！ |
kutimiti1234
left a comment
kutimiti1234
left a comment
There was a problem hiding this comment.
大変お待たせしたのに細かいところしか指摘点なく、申し訳ございません。
挙動としては特に問題ある部分見受けられないので、下記2点だけ検討いただけますと幸いです。
| constructor(practices, setPracticeId, userPracticeId) { | ||
| this.practices = practices | ||
| this.setPracticeId = setPracticeId | ||
| this.userPracticeId = userPracticeId |
There was a problem hiding this comment.
ここだけではないのですが、userは不要に感じました。companiesのreportsページにもプルダウンを設置する可能性があるため。
There was a problem hiding this comment.
変数userPracticeId のuser が不要という変数名の指摘で合ってますでしょうか？
指摘点の認識が正しい場合、修正いたします！
元の変数名を踏襲したものにしていましたが、確かに分かりづらい変数名かもしれないと感じました。
There was a problem hiding this comment.
userは不要に感じました。
user を削除しました。
また、記述位置を入れ替えました。
| ).default | ||
| dropdown = new PracticeFilterDropdown( | ||
| practices, | ||
| setUserPracticeId, |
There was a problem hiding this comment.
ここもcompaniesにdropdownを置く可能性もあるので、不要に感じました。
There was a problem hiding this comment.
こちらもsetUserPracticeId 変数名のUser が不要という変数名の指摘と認識しましたが正しいでしょうか？
There was a problem hiding this comment.
user だと「ユーザーのプラクティス」というニュアンスになってしまうので、「選択したプラクティス」という意味合いでseledted に変更しました。
単純にpractice に変更すると既存のprops名と重複してしまうのを避ける意図もあります。
karlley
left a comment
karlley
left a comment
There was a problem hiding this comment.
@kutimiti1234
レビューありがとうございます！
指摘点に関して確認の質問を追加しましたので確認お願いします🙇♂️
| constructor(practices, setPracticeId, userPracticeId) { | ||
| this.practices = practices | ||
| this.setPracticeId = setPracticeId | ||
| this.userPracticeId = userPracticeId |
There was a problem hiding this comment.
変数userPracticeId のuser が不要という変数名の指摘で合ってますでしょうか？
指摘点の認識が正しい場合、修正いたします！
元の変数名を踏襲したものにしていましたが、確かに分かりづらい変数名かもしれないと感じました。
| ).default | ||
| dropdown = new PracticeFilterDropdown( | ||
| practices, | ||
| setUserPracticeId, |
There was a problem hiding this comment.
こちらもsetUserPracticeId 変数名のUser が不要という変数名の指摘と認識しましたが正しいでしょうか？
はい、どちらも |
karlley
force-pushed
the
chore/convert-practice-filter-dropdown-to-vanilla-js
branch
from
da98019 to
acb7788
Compare
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 (3)
app/javascript/components/PracticeFilterDropdown.jsx(0 hunks)app/javascript/components/Reports.jsx(3 hunks)app/javascript/practice-filter-dropdown.js(1 hunks)
💤 Files with no reviewable changes (1)
- app/javascript/components/PracticeFilterDropdown.jsx
🧰 Additional context used
📓 Path-based instructions (3)
app/**/*.{rb,js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Rails app code should be organized in
app/directory with subdirectories:models/,controllers/,views/,jobs/,helpers/, and frontend code underjavascript/(Shakapacker)
Files:
app/javascript/practice-filter-dropdown.jsapp/javascript/components/Reports.jsx
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/practice-filter-dropdown.jsapp/javascript/components/Reports.jsx
**/*.js
⚙️ CodeRabbit configuration file
**/*.js: - どうしても避けられない時以外に新規にVue.js, Reactのコードを追加しない。
- fetchメソッドの代わりにrequest.jsを使う。
Files:
app/javascript/practice-filter-dropdown.js
🧠 Learnings (18)
📓 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: sjabcdefin
Repo: fjordllc/bootcamp PR: 9090
File: app/javascript/user-sns.js:2-25
Timestamp: 2025-08-25T08:00:11.369Z
Learning: app/javascript/user-sns.js において、アクセシビリティ改善（aria-label属性の追加など）は他の箇所との整合性を保つため、別途統一して対応する方針である
📚 Learning: 2025-11-13T20:44:13.908Z
Learnt from: karlley
Repo: fjordllc/bootcamp PR: 9304
File: app/javascript/practice-filter-dropdown.js:3-3
Timestamp: 2025-11-13T20:44:13.908Z
Learning: fjordllc/bootcampプロジェクトでは、JavaScriptファイルでクラスをエクスポートする際に`export default class {`のパターン（無名クラス）を一貫して使用している。これは複数のファイル（textarea-autocomplte-mention.js、textarea-autocomplte-emoji.js、user-icon-renderer.js等）で確認されており、プロジェクトのコーディング規約となっている。
Applied to files:
app/javascript/practice-filter-dropdown.js
📚 Learning: 2025-06-29T03:44:15.179Z
Learnt from: hirokiej
Repo: fjordllc/bootcamp PR: 8740
File: app/helpers/reports_helper.rb:56-64
Timestamp: 2025-06-29T03:44:15.179Z
Learning: このプロジェクト（fjordllc/bootcamp）では、ja.ymlファイルで一部の単語や文章のみI18n対応されているが、多くのテキストは日本語でハードコーディングされており、完全な国際化対応は行われていない。新しい機能でもI18n対応は不要と判断される。
Applied to files:
app/javascript/practice-filter-dropdown.jsapp/javascript/components/Reports.jsx
📚 Learning: 2025-07-23T20:42:19.974Z
Learnt from: jun-kondo
Repo: fjordllc/bootcamp PR: 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/javascript/practice-filter-dropdown.js
📚 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/practice-filter-dropdown.jsapp/javascript/components/Reports.jsx
📚 Learning: 2025-11-17T00:46:30.794Z
Learnt from: tyrrell-IH
Repo: fjordllc/bootcamp PR: 9306
File: app/javascript/components/Bookmarks.jsx:248-265
Timestamp: 2025-11-17T00:46:30.794Z
Learning: fjordllc/bootcamp プロジェクトでは、Reactからバニラ JavaScript への移行作業が進行中である。そのため、新しいReactファイルの作成は避け、既存のReactコンポーネント内で完結する実装が推奨される。BookmarksInDashboard.jsx は issue #9045 で削除予定。
Applied to files:
app/javascript/practice-filter-dropdown.jsapp/javascript/components/Reports.jsx
📚 Learning: 2025-09-12T21:16:47.639Z
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のスコープ維持が重要。
Applied to files:
app/javascript/practice-filter-dropdown.jsapp/javascript/components/Reports.jsx
📚 Learning: 2025-07-30T07:26:36.599Z
Learnt from: ryufuta
Repo: fjordllc/bootcamp PR: 8992
File: app/models/correct_answer_notifier.rb:23-23
Timestamp: 2025-07-30T07:26:36.599Z
Learning: PRのスコープを維持することが重要であり、バグ修正PRにおいて、修正対象以外の機能（例：ルーティングテスト）の追加提案は適切ではない。そのような改善が必要な場合は別PRで対応すべきである。
Applied to files:
app/javascript/practice-filter-dropdown.js
📚 Learning: 2025-09-11T14:47:53.256Z
Learnt from: komagata
Repo: fjordllc/bootcamp PR: 9101
File: app/models/product.rb:59-61
Timestamp: 2025-09-11T14:47:53.256Z
Learning: Rails アップグレードPRでは、アップグレードに直接関連しない性能改善や機能追加の提案は避けるべき。PRのスコープを維持することが重要で、そのような改善提案は別PRで対応すべきである。
Applied to files:
app/javascript/practice-filter-dropdown.js
📚 Learning: 2025-09-12T21:18:00.834Z
Learnt from: komagata
Repo: fjordllc/bootcamp PR: 9101
File: app/notifiers/discord_notifier.rb:131-135
Timestamp: 2025-09-12T21:18:00.834Z
Learning: Rails アップグレードPRにおいて、product_review_not_completed メソッドの webhook URL 設定の shared フォールバック追加も、設定システム全体の変更として別PRで対応すべきである。
Applied to files:
app/javascript/practice-filter-dropdown.jsapp/javascript/components/Reports.jsx
📚 Learning: 2025-09-05T03:33:31.659Z
Learnt from: sharoa119
Repo: fjordllc/bootcamp PR: 9012
File: db/migrate/20250905025850_remove_redundant_index_from_reports.rb:5-5
Timestamp: 2025-09-05T03:33:31.659Z
Learning: reports テーブルには既存の一意制約インデックス index_reports_on_user_id_and_reported_on があるため、同じカラム組み合わせ (user_id, reported_on) の非一意インデックスは冗長であり削除が適切。
Applied to files:
app/javascript/practice-filter-dropdown.js
📚 Learning: 2025-11-19T07:55:51.504Z
Learnt from: smallmonkeykey
Repo: fjordllc/bootcamp PR: 9314
File: app/helpers/page_tabs/dashboard_helper.rb:8-12
Timestamp: 2025-11-19T07:55:51.504Z
Learning: fjordllc/bootcamp プロジェクトにおいて、分報（micro_reports）は current_user 専用の機能ではなく、他ユーザーの分報も閲覧・管理する UI で使われているため、ダッシュボードの「自分の分報」タブでは user_micro_reports_path(current_user, ...) を使用し、/users/:id/micro_reports の URL 構造を維持する。current_user 名前空間に新しいルートを追加しない方針である。
Applied to files:
app/javascript/practice-filter-dropdown.js
📚 Learning: 2025-09-05T03:33:31.659Z
Learnt from: sharoa119
Repo: fjordllc/bootcamp PR: 9012
File: db/migrate/20250905025850_remove_redundant_index_from_reports.rb:5-5
Timestamp: 2025-09-05T03:33:31.659Z
Learning: reports テーブルには既存の一意制約インデックス index_reports_on_user_id_and_reported_on があるため、同じカラム組み合わせ (user_id, reported_on) の非一意インデックスは冗長であり削除が適切。一意制約インデックスは非一意インデックスと同等かそれ以上のクエリ性能を提供する。
Applied to files:
app/javascript/practice-filter-dropdown.js
📚 Learning: 2025-08-25T07:23:54.802Z
Learnt from: sjabcdefin
Repo: fjordllc/bootcamp PR: 9090
File: app/javascript/user_sns.js:0-0
Timestamp: 2025-08-25T07:23:54.802Z
Learning: app/javascript/user_sns.js の会社ロゴ表示において、_list_user.json.builder の構造により user.company.logo_url が存在する場合は必ず user.company.url も存在することが API レスポンス構造で保証されている
Applied to files:
app/javascript/practice-filter-dropdown.js
📚 Learning: 2025-09-05T03:16:53.387Z
Learnt from: sharoa119
Repo: fjordllc/bootcamp PR: 9012
File: app/queries/grass_learning_time_query.rb:30-37
Timestamp: 2025-09-05T03:16:53.387Z
Learning: reports テーブルには既に (user_id, reported_on) の一意制約インデックス index_reports_on_user_id_and_reported_on が存在するため、学習時間集計クエリのパフォーマンス最適化には追加のインデックスは不要。
Applied to files:
app/javascript/practice-filter-dropdown.js
📚 Learning: 2025-08-31T03:39:07.792Z
Learnt from: mousu-a
Repo: fjordllc/bootcamp PR: 8566
File: app/views/pair_works/_body.html.slim:85-85
Timestamp: 2025-08-31T03:39:07.792Z
Learning: app/views/pair_works/_body.html.slimテンプレートには2つの独立したif pair_work.solved?条件分岐があり、user変数は最初の分岐でのみ設定されるため、2番目の分岐での参照には注意が必要。
Applied to files:
app/javascript/practice-filter-dropdown.js
📚 Learning: 2025-09-01T22:31:57.345Z
Learnt from: matuaya
Repo: fjordllc/bootcamp PR: 9108
File: app/models/after_user_retirement.rb:0-0
Timestamp: 2025-09-01T22:31:57.345Z
Learning: fjordllc/bootcampプロジェクトにおいて、`Newspaper.publish(:retirement_create, { user: user })`はUnfinishedDataDestroyer（未チェックProduct・WIP Report削除、career_pathリセット）とTimesChannelDestroyer（Discord分報チャンネル削除）による重要なデータクリーンアップ処理を実行するため、退会のトリガーが何であろうと（admin、hibernation、user）必ず実行する必要がある。
Applied to files:
app/javascript/components/Reports.jsx
📚 Learning: 2025-09-01T22:31:57.345Z
Learnt from: matuaya
Repo: fjordllc/bootcamp PR: 9108
File: app/models/after_user_retirement.rb:0-0
Timestamp: 2025-09-01T22:31:57.345Z
Learning: fjordllc/bootcampプロジェクトにおいて、`Newspaper.publish(:retirement_create, { user: user })`は通知処理ではなく、UnfinishedDataDestroyerやTimesChannelDestroyer等による重要なデータクリーンアップ処理を実行するため、管理者による退会（triggered_by: 'admin'）の場合でも必ず実行する必要がある。
Applied to files:
app/javascript/components/Reports.jsx
🧬 Code graph analysis (1)
app/javascript/practice-filter-dropdown.js (1)
app/javascript/components/Reports.jsx (1)
practiceId(21-21)
⏰ 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/practice-filter-dropdown.js (1)
1-76: バニラJSへの移行が適切に実装されています以前のレビューで指摘されたイベントリスナーのクリーンアップ問題が修正され、メモリリーク対策が適切に実装されています。Choices.jsの初期化、オプションの動的生成、destroy()でのクリーンアップなど、全体的な実装は問題ありません。
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/javascript/components/Reports.jsx (2)
32-63: PracticeFilterDropdown の非同期初期化とクリーンアップは概ね良さそうです
isMountedフラグとクリーンアップ内でのdestroy()呼び出しにより、アンマウント後にインスタンスが残る問題や非同期インポート後のレースはきちんと防げているように見えますし、依存配列からpracticeIdを外して Choices 側で状態を持たせる前提の設計も妥当だと思います。
data変更のたびにドロップダウンが作り直される挙動が気になるようであれば、useRefでインスタンスを保持して「未初期化時のみ init する」ようにする最適化も検討の余地はありますが、この PR のスコープでは必須ではないと感じました。
81-88: プレースホルダ要素の配置も問題ないですが、将来の再利用時は ref の利用も検討ください
totalPages === 0/> 0の両方の分岐でpracticesがある場合に<div data-practice-filter-dropdown>を出しているので、常に一覧の有無にかかわらずドロップダウンが表示される形になっていて良さそうです。一方で、現在は
document.querySelector('[data-practice-filter-dropdown]')で最初の 1 要素を取る実装なので、将来Reportsコンポーネントが 1 画面に複数マウントされるようなケースが出てきた場合は、refで特定の要素に紐付ける実装に変えておくとより安全だと思います。
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/javascript/components/Reports.jsx(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
app/**/*.{rb,js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Rails app code should be organized in
app/directory with subdirectories:models/,controllers/,views/,jobs/,helpers/, and frontend code underjavascript/(Shakapacker)
Files:
app/javascript/components/Reports.jsx
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/components/Reports.jsx
🧠 Learnings (8)
📓 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: sjabcdefin
Repo: fjordllc/bootcamp PR: 9090
File: app/javascript/user-sns.js:2-25
Timestamp: 2025-08-25T08:00:11.369Z
Learning: app/javascript/user-sns.js において、アクセシビリティ改善（aria-label属性の追加など）は他の箇所との整合性を保つため、別途統一して対応する方針である
📚 Learning: 2025-09-01T22:31:57.345Z
Learnt from: matuaya
Repo: fjordllc/bootcamp PR: 9108
File: app/models/after_user_retirement.rb:0-0
Timestamp: 2025-09-01T22:31:57.345Z
Learning: fjordllc/bootcampプロジェクトにおいて、`Newspaper.publish(:retirement_create, { user: user })`はUnfinishedDataDestroyer（未チェックProduct・WIP Report削除、career_pathリセット）とTimesChannelDestroyer（Discord分報チャンネル削除）による重要なデータクリーンアップ処理を実行するため、退会のトリガーが何であろうと（admin、hibernation、user）必ず実行する必要がある。
Applied to files:
app/javascript/components/Reports.jsx
📚 Learning: 2025-09-12T21:18:00.834Z
Learnt from: komagata
Repo: fjordllc/bootcamp PR: 9101
File: app/notifiers/discord_notifier.rb:131-135
Timestamp: 2025-09-12T21:18:00.834Z
Learning: Rails アップグレードPRにおいて、product_review_not_completed メソッドの webhook URL 設定の shared フォールバック追加も、設定システム全体の変更として別PRで対応すべきである。
Applied to files:
app/javascript/components/Reports.jsx
📚 Learning: 2025-09-01T22:31:57.345Z
Learnt from: matuaya
Repo: fjordllc/bootcamp PR: 9108
File: app/models/after_user_retirement.rb:0-0
Timestamp: 2025-09-01T22:31:57.345Z
Learning: fjordllc/bootcampプロジェクトにおいて、`Newspaper.publish(:retirement_create, { user: user })`は通知処理ではなく、UnfinishedDataDestroyerやTimesChannelDestroyer等による重要なデータクリーンアップ処理を実行するため、管理者による退会（triggered_by: 'admin'）の場合でも必ず実行する必要がある。
Applied to files:
app/javascript/components/Reports.jsx
📚 Learning: 2025-11-17T00:46:30.794Z
Learnt from: tyrrell-IH
Repo: fjordllc/bootcamp PR: 9306
File: app/javascript/components/Bookmarks.jsx:248-265
Timestamp: 2025-11-17T00:46:30.794Z
Learning: fjordllc/bootcamp プロジェクトでは、Reactからバニラ JavaScript への移行作業が進行中である。そのため、新しいReactファイルの作成は避け、既存のReactコンポーネント内で完結する実装が推奨される。BookmarksInDashboard.jsx は issue #9045 で削除予定。
Applied to files:
app/javascript/components/Reports.jsx
📚 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/components/Reports.jsx
📚 Learning: 2025-09-12T21:16:47.639Z
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のスコープ維持が重要。
Applied to files:
app/javascript/components/Reports.jsx
📚 Learning: 2025-06-29T03:44:15.179Z
Learnt from: hirokiej
Repo: fjordllc/bootcamp PR: 8740
File: app/helpers/reports_helper.rb:56-64
Timestamp: 2025-06-29T03:44:15.179Z
Learning: このプロジェクト（fjordllc/bootcamp）では、ja.ymlファイルで一部の単語や文章のみI18n対応されているが、多くのテキストは日本語でハードコーディングされており、完全な国際化対応は行われていない。新しい機能でもI18n対応は不要と判断される。
Applied to files:
app/javascript/components/Reports.jsx
⏰ 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
karlley
force-pushed
the
chore/convert-practice-filter-dropdown-to-vanilla-js
branch
2 times, most recently
from
a75be10 to
6e16973
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/javascript/practice-filter-dropdown.js (1)
3-11: コンストラクタ引数の末尾カンマについて
constructor(practices, practiceId, setPracticeId,)の末尾カンマはモダンJSとしては文法的に問題ありませんが、ESLint/Prettier の設定によってはスタイル違反として弾かれる可能性があります。既存ファイルのスタイルに合わせて末尾カンマを削除しておくと安全だと思います。
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/javascript/components/PracticeFilterDropdown.jsx(0 hunks)app/javascript/components/Reports.jsx(3 hunks)app/javascript/practice-filter-dropdown.js(1 hunks)
💤 Files with no reviewable changes (1)
- app/javascript/components/PracticeFilterDropdown.jsx
🚧 Files skipped from review as they are similar to previous changes (1)
- app/javascript/components/Reports.jsx
🧰 Additional context used
📓 Path-based instructions (3)
app/**/*.{rb,js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Rails app code should be organized in
app/directory with subdirectories:models/,controllers/,views/,jobs/,helpers/, and frontend code underjavascript/(Shakapacker)
Files:
app/javascript/practice-filter-dropdown.js
app/javascript/**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
JavaScript/TypeScript code in
app/javascript/should be linted with ESLint and formatted with Prettier viayarn lintscripts; use React 17 and Shakapacker/Webpack 5
Files:
app/javascript/practice-filter-dropdown.js
**/*.js
⚙️ CodeRabbit configuration file
**/*.js: - どうしても避けられない時以外に新規にVue.js, Reactのコードを追加しない。
- fetchメソッドの代わりにrequest.jsを使う。
Files:
app/javascript/practice-filter-dropdown.js
🧠 Learnings (16)
📓 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: sjabcdefin
Repo: fjordllc/bootcamp PR: 9090
File: app/javascript/user-sns.js:2-25
Timestamp: 2025-08-25T08:00:11.369Z
Learning: app/javascript/user-sns.js において、アクセシビリティ改善（aria-label属性の追加など）は他の箇所との整合性を保つため、別途統一して対応する方針である
📚 Learning: 2025-11-13T20:44:13.908Z
Learnt from: karlley
Repo: fjordllc/bootcamp PR: 9304
File: app/javascript/practice-filter-dropdown.js:3-3
Timestamp: 2025-11-13T20:44:13.908Z
Learning: fjordllc/bootcampプロジェクトでは、JavaScriptファイルでクラスをエクスポートする際に`export default class {`のパターン（無名クラス）を一貫して使用している。これは複数のファイル（textarea-autocomplte-mention.js、textarea-autocomplte-emoji.js、user-icon-renderer.js等）で確認されており、プロジェクトのコーディング規約となっている。
Applied to files:
app/javascript/practice-filter-dropdown.js
📚 Learning: 2025-06-29T03:44:15.179Z
Learnt from: hirokiej
Repo: fjordllc/bootcamp PR: 8740
File: app/helpers/reports_helper.rb:56-64
Timestamp: 2025-06-29T03:44:15.179Z
Learning: このプロジェクト（fjordllc/bootcamp）では、ja.ymlファイルで一部の単語や文章のみI18n対応されているが、多くのテキストは日本語でハードコーディングされており、完全な国際化対応は行われていない。新しい機能でもI18n対応は不要と判断される。
Applied to files:
app/javascript/practice-filter-dropdown.js
📚 Learning: 2025-07-23T20:42:19.974Z
Learnt from: jun-kondo
Repo: fjordllc/bootcamp PR: 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/javascript/practice-filter-dropdown.js
📚 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/practice-filter-dropdown.js
📚 Learning: 2025-11-17T00:46:30.794Z
Learnt from: tyrrell-IH
Repo: fjordllc/bootcamp PR: 9306
File: app/javascript/components/Bookmarks.jsx:248-265
Timestamp: 2025-11-17T00:46:30.794Z
Learning: fjordllc/bootcamp プロジェクトでは、Reactからバニラ JavaScript への移行作業が進行中である。そのため、新しいReactファイルの作成は避け、既存のReactコンポーネント内で完結する実装が推奨される。BookmarksInDashboard.jsx は issue #9045 で削除予定。
Applied to files:
app/javascript/practice-filter-dropdown.js
📚 Learning: 2025-09-12T21:16:47.639Z
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のスコープ維持が重要。
Applied to files:
app/javascript/practice-filter-dropdown.js
📚 Learning: 2025-07-30T07:26:36.599Z
Learnt from: ryufuta
Repo: fjordllc/bootcamp PR: 8992
File: app/models/correct_answer_notifier.rb:23-23
Timestamp: 2025-07-30T07:26:36.599Z
Learning: PRのスコープを維持することが重要であり、バグ修正PRにおいて、修正対象以外の機能（例：ルーティングテスト）の追加提案は適切ではない。そのような改善が必要な場合は別PRで対応すべきである。
Applied to files:
app/javascript/practice-filter-dropdown.js
📚 Learning: 2025-09-11T14:47:53.256Z
Learnt from: komagata
Repo: fjordllc/bootcamp PR: 9101
File: app/models/product.rb:59-61
Timestamp: 2025-09-11T14:47:53.256Z
Learning: Rails アップグレードPRでは、アップグレードに直接関連しない性能改善や機能追加の提案は避けるべき。PRのスコープを維持することが重要で、そのような改善提案は別PRで対応すべきである。
Applied to files:
app/javascript/practice-filter-dropdown.js
📚 Learning: 2025-09-12T21:18:00.834Z
Learnt from: komagata
Repo: fjordllc/bootcamp PR: 9101
File: app/notifiers/discord_notifier.rb:131-135
Timestamp: 2025-09-12T21:18:00.834Z
Learning: Rails アップグレードPRにおいて、product_review_not_completed メソッドの webhook URL 設定の shared フォールバック追加も、設定システム全体の変更として別PRで対応すべきである。
Applied to files:
app/javascript/practice-filter-dropdown.js
📚 Learning: 2025-09-05T03:33:31.659Z
Learnt from: sharoa119
Repo: fjordllc/bootcamp PR: 9012
File: db/migrate/20250905025850_remove_redundant_index_from_reports.rb:5-5
Timestamp: 2025-09-05T03:33:31.659Z
Learning: reports テーブルには既存の一意制約インデックス index_reports_on_user_id_and_reported_on があるため、同じカラム組み合わせ (user_id, reported_on) の非一意インデックスは冗長であり削除が適切。
Applied to files:
app/javascript/practice-filter-dropdown.js
📚 Learning: 2025-11-19T07:55:51.504Z
Learnt from: smallmonkeykey
Repo: fjordllc/bootcamp PR: 9314
File: app/helpers/page_tabs/dashboard_helper.rb:8-12
Timestamp: 2025-11-19T07:55:51.504Z
Learning: fjordllc/bootcamp プロジェクトにおいて、分報（micro_reports）は current_user 専用の機能ではなく、他ユーザーの分報も閲覧・管理する UI で使われているため、ダッシュボードの「自分の分報」タブでは user_micro_reports_path(current_user, ...) を使用し、/users/:id/micro_reports の URL 構造を維持する。current_user 名前空間に新しいルートを追加しない方針である。
Applied to files:
app/javascript/practice-filter-dropdown.js
📚 Learning: 2025-09-05T03:33:31.659Z
Learnt from: sharoa119
Repo: fjordllc/bootcamp PR: 9012
File: db/migrate/20250905025850_remove_redundant_index_from_reports.rb:5-5
Timestamp: 2025-09-05T03:33:31.659Z
Learning: reports テーブルには既存の一意制約インデックス index_reports_on_user_id_and_reported_on があるため、同じカラム組み合わせ (user_id, reported_on) の非一意インデックスは冗長であり削除が適切。一意制約インデックスは非一意インデックスと同等かそれ以上のクエリ性能を提供する。
Applied to files:
app/javascript/practice-filter-dropdown.js
📚 Learning: 2025-08-25T07:23:54.802Z
Learnt from: sjabcdefin
Repo: fjordllc/bootcamp PR: 9090
File: app/javascript/user_sns.js:0-0
Timestamp: 2025-08-25T07:23:54.802Z
Learning: app/javascript/user_sns.js の会社ロゴ表示において、_list_user.json.builder の構造により user.company.logo_url が存在する場合は必ず user.company.url も存在することが API レスポンス構造で保証されている
Applied to files:
app/javascript/practice-filter-dropdown.js
📚 Learning: 2025-09-05T03:16:53.387Z
Learnt from: sharoa119
Repo: fjordllc/bootcamp PR: 9012
File: app/queries/grass_learning_time_query.rb:30-37
Timestamp: 2025-09-05T03:16:53.387Z
Learning: reports テーブルには既に (user_id, reported_on) の一意制約インデックス index_reports_on_user_id_and_reported_on が存在するため、学習時間集計クエリのパフォーマンス最適化には追加のインデックスは不要。
Applied to files:
app/javascript/practice-filter-dropdown.js
📚 Learning: 2025-08-31T03:39:07.792Z
Learnt from: mousu-a
Repo: fjordllc/bootcamp PR: 8566
File: app/views/pair_works/_body.html.slim:85-85
Timestamp: 2025-08-31T03:39:07.792Z
Learning: app/views/pair_works/_body.html.slimテンプレートには2つの独立したif pair_work.solved?条件分岐があり、user変数は最初の分岐でのみ設定されるため、2番目の分岐での参照には注意が必要。
Applied to files:
app/javascript/practice-filter-dropdown.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 (2)
app/javascript/practice-filter-dropdown.js (2)
13-55: DOM構築と初期選択ロジックはReact版と同等の挙動に見えます
- ラベルと
<select>の構造、デフォルトの「全ての日報を表示」オプション、practicesからの<option>生成はいずれも素直で問題なさそうです。this.practiceIdが指定されているときだけsetChoiceByValueで初期選択する実装も、従来コンポーネントの挙動と整合しているように見えます。この部分については特に懸念ありません。
57-75: changeイベントリスナーとdestroyのクリーンアップが適切です
this.handleSelectChangeをインスタンスプロパティに保持し、destroy()でremoveEventListenerとchoices.destroy()を呼んだ後に参照をnullに戻しているので、前回指摘されていたメモリリークの懸念は解消されていると思います。
karlley
force-pushed
the
chore/convert-practice-filter-dropdown-to-vanilla-js
branch
from
6e16973 to
1ca70fd
Compare
#9304 (comment) 確認お願いします！ |
kutimiti1234
left a comment
kutimiti1234
left a comment
There was a problem hiding this comment.
ご対応ありがとうございます
approveさせていただきます。
|
@kutimiti1234 @okuramasafumi |
| useEffect(() => { | ||
| if (!data || !practices) return | ||
|
|
||
| let isMounted = true |
There was a problem hiding this comment.
isMountedの機能（重複作成を防ぐためという認識）はdropdownのnullチェックで代用できたら変数を一つ減らせそうだと思いました。この場合、dropdown.destroy()の後でdropdown = nullする必要はありますね。
There was a problem hiding this comment.
dropdownのnullチェックで代用できたら変数を一つ減らせそう
確かにそうですね、不要でした。
isMounted を使用しないように修正しました。
karlley
force-pushed
the
chore/convert-practice-filter-dropdown-to-vanilla-js
branch
from
1ca70fd to
8264207
Compare
|
レビューありがとうございます！ |
okuramasafumi
left a comment
okuramasafumi
left a comment
There was a problem hiding this comment.
レビュー遅くなってしまってすみません、LTGM!
|
@okuramasafumi @komagata |
komagata
deleted the
chore/convert-practice-filter-dropdown-to-vanilla-js
branch
|
@karlley マージしました〜 |
4 participants
Issue
概要
変更確認方法
chore/convert-practice-filter-dropdown-to-vanilla-jsをローカルに取り込む各項目ごとに以下を確認
絞り込み機能
komagataでログインkomagataを選択し、日報タブを開く(/users/:id/reports)全ての日報を表示になっているOS X Mountain Lionをクリーンインストールするを選択すると該当の日報が絞り込まれるTerminalの基礎を覚えるを選択すると日報がヒットしない(存在しないため)全ての日報を表示を選択するとkomagataの全日報が表示されるレスポンシブ
komagataでログインkomagataを選択し、日報タブを開く(/users/:id/reports)Screenshot
画面上の変更は無し
Summary by CodeRabbit
新機能
リファクタリング
削除
✏️ Tip: You can customize this high-level summary in your review settings.