0
✦ 品質
🛡 セキュリティ
⚙ 保守性
📋 概要
Ruby on Rails 7 + Stimulus.js で構築された育成型タスク管理Webアプリ「RewardMe」。ミニゲーム6種・ガチャ・マイルーム・リボンちゃん育成など多機能なゲームロジックをフルスクラッチ実装しており、completed_countを唯一のソース・オブ・トゥルースとする設計が特徴。
🎯 今すぐ直すべき3点
-
app/javascript/controllers/match_game_controller.js デバッグ用URLパラメータ(?fx=nostar,nozoom等)を本番コードに残したままにしている。connect()内の `const fx = new URLSearchParams(location.search).get('fx')` ブロックを削除し、本番環境でのデバッグ用コードパスを完全に除去する↗
-
app/javascript/controllers/puzzle_controller.js selectPuzzle()のfetchエラーハンドリングが `catch { /* silent */ }` で完全に握り潰されており、ネットワーク障害・CSRF失敗時にユーザーへ一切フィードバックが届かない。catchブロックでエラーメッセージをUIに表示する処理を追加する↗
-
app/models/user.rb badge・ribbon_level・ribbon_stage・ribbon_exp等のゲームロジックメソッド群がUserモデルに直接実装されており、単一責任原則に違反している。RibbonProgressionやUserBadgeCalculatorなどのPORO(Plain Old Ruby Object)またはConcernに切り出し、Userモデルの肥大化を解消する↗
🔍 検出された問題 (7件)
🟧 Warning
📄 app/javascript/controllers/match_game_controller.js
本番コードにデバッグ用URLパラメータ制御が残存
match_game_controller.js の connect() 内で `?fx=nostar,nozoom` 等のURLパラメータを読み取り、document.documentElement.dataset.mgFx に書き込む処理が本番コードに残っている。悪意あるユーザーがURLパラメータを操作してDOM属性を書き換えられる状態であり、意図しないCSS/JS挙動の変化やデバッグ情報の露出につながる危険がある
🟧 Warning
📄 app/javascript/controllers/puzzle_controller.js
fetchエラーの完全な握り潰しによるサイレント障害
puzzle_controller.js の selectPuzzle() が `catch { /* silent */ }` でネットワークエラー・CSRF検証失敗・サーバーエラーをすべて無視している。ユーザーはパズル選択が失敗したことを知る手段がなく、不整合な状態でゲームを開始してしまう可能性がある
🟧 Warning
📄 app/models/user.rb
Userモデルへのゲームロジック集中による責務過多
app/models/user.rb にbadge・ribbon_level・ribbon_stage・ribbon_exp・ribbon_exp_percent・next_stage_tasks・ribbon_message・ribbon_stage_image・benny_image・growth_room?・room_growth_stage等のゲーム進行ロジックが直接実装されている。ファイル全体が11073文字(truncated)に達しており、ドメインロジックとActiveRecordの責務が混在することで、テスト・変更・再利用が困難になっている
🟧 Warning
📄 app/javascript/controllers/match_game_controller.js
複数のStimulusコントローラーが50行を大幅に超える巨大実装
match_game_controller.js(24289文字)・potion_game_controller.js(22552文字)・flight_game_controller.js(13900文字)がいずれも単一ファイルに全ゲームロジックを収めており、ゲーム状態管理・描画・入力処理・サーバー通信が混在している。変更時の影響範囲が広く、バグの局所化が困難
🟧 Warning
📄 app/javascript/controllers/flight_game_controller.js
flight_game_controller.js でのCanvas描画エラーハンドリング欠如
flight_game_controller.js の connect() で画像ロード(bg.onload / benny.onload)に onerror ハンドラが設定されておらず、画像取得失敗時にゲームが無音で壊れた状態で動作し続ける。ユーザーへのフォールバック表示がない
🟨 Suggestion
📄 app/models/letter.rb
app/models/letter.rb がActiveRecordを継承しないPORO設計だが型安全性がない
Letter クラスはCATALOGハッシュ配列をデータストアとして持つ設計で、DBアクセスなしに手紙データを管理している点は軽量だが、triggerやreceived_atの型・値域チェックが一切なく、ハッシュキーのタイポが実行時まで検出されない。Structやデータクラスへの移行を検討すべき
🟨 Suggestion
📄 app/javascript/controllers/ribbon_controller.js
ribbon_controller.js のROOM_MESSAGES・SPECIAL_COMBOSが肥大化しメンテナンス困難
ribbon_controller.js にROOM_MESSAGES(18部屋分)とSPECIAL_COMBOSが直書きされており、ファイルが7251文字に達している。セリフの追加・修正のたびにJSファイルを変更する必要があり、将来的にi18n対応や動的セリフ管理が困難になる
✅ 良い点
- app/models/user.rb の create_guest! / cleanup_guests! メソッドが、ゲストユーザーのライフサイクル(生成・自動削除)を明確に分離しており、SecureRandom.hex による推測不能なemail/password生成でゲスト認証の安全性を担保している
- Dockerfile がマルチステージビルド(builder/runtime分離)を採用しており、ビルド時のダミーSECRET_KEY_BASEが最終イメージに残らない設計になっている点が、シークレット漏洩リスクを構造的に排除している
- app/javascript/controllers/match_game_controller.js の状態機械(S.idle/S.swapping/S.resolving)とアニメーション定数Tの集約により、ゲームの入力制御とタイミング管理が一元化されており、競合状態(race condition)を防ぐ設計になっている
🔧 リファクタ提案
- app/models/user.rb のリボン育成ロジック(ribbon_stage・ribbon_exp・ribbon_exp_percent・next_stage_tasks等)をapp/models/concerns/ribbon_progression.rbまたはapp/services/ribbon_progression_calculator.rbに切り出し、UserモデルはActiveRecordの責務のみに絞る
- match_game_controller.js・potion_game_controller.js・flight_game_controller.jsのゲームロジックを、状態管理クラス(例: MatchGameState)・描画クラス(例: MatchGameRenderer)・入力ハンドラに分割し、Stimulusコントローラーはそれらの協調のみを担うアーキテクチャに移行する
- ribbon_controller.js のROOM_MESSAGESとSPECIAL_COMBOSをJSONファイル(app/javascript/data/room_messages.json)に外出しし、importで読み込む形にすることで、セリフ管理とロジックを分離する
🔒 セキュリティ注意点
- puzzle_controller.js のfetchリクエストでCSRFトークンを `document.querySelector('meta[name="csrf-token"]').content` から取得しているが、querySelector が null を返した場合(CSPやDOM操作による除去)に例外が発生しリクエストが中断する。オプショナルチェーン(?.content)と明示的なエラー処理を追加すること
- app/models/user.rb の cleanup_guests! が `where(is_guest: true).where(created_at: ...older_than.ago).destroy_all` を使用しており、dependent: :destroyによるコールバック実行は正しいが、大量ゲストユーザーが蓄積した場合にN+1的なdestroyループでパフォーマンス問題が発生しうる。バッチサイズを制限した定期実行(例: find_each + destroy)への変更を検討すること
📤 PRコメント出力
このレビューをGitHub PRコメントとして貼り付けられるMarkdown形式で出力します。